New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for running IOMCU FW on CubeRedSecondary #29094

Open

bugobliterator wants to merge 19 commits into ArduPilot:master from bugobliterator:pr-cubered-iomcu

+541 −92

Member

bugobliterator commented Jan 17, 2025 •

edited

Loading

Tested working DSHOT, PWM modes and RC Input on CubredSecondary with IOFirmware.

No size changes for older iomcus, checked using sizecompare for both with and without dshot.

Allows updates to the secondary MCU's firmware even after the IOFirmware is flashed.

bugobliterator marked this pull request as draft

January 17, 2025 00:33

bugobliterator force-pushed the pr-cubered-iomcu branch 3 times, most recently from ed65847 to 2647c5d Compare

January 17, 2025 04:22

bugobliterator force-pushed the pr-cubered-iomcu branch 2 times, most recently from dfc80b2 to 100901b Compare

January 28, 2025 01:52

bugobliterator requested review from andyp1per and tridge and removed request for andyp1per

January 29, 2025 03:52

bugobliterator marked this pull request as ready for review

January 29, 2025 03:52

bugobliterator force-pushed the pr-cubered-iomcu branch from 0a9b94d to 78d8f69 Compare

January 29, 2025 04:03

andyp1per requested changes

View reviewed changes

Collaborator

andyp1per left a comment

Looking good. Some small changes. Think you need to think about how it looks if someone adds F4 support (which has been requested)

libraries/AP_HAL_ChibiOS/Scheduler.cpp Outdated Show resolved Hide resolved

libraries/AP_IOMCU/AP_IOMCU.cpp Outdated Show resolved Hide resolved

libraries/AP_IOMCU/fw_uploader.cpp Outdated

@@ @@ -281,7 +290,7 @@ bool AP_IOMCU::erase() @@
                   debug("erase...");
                   send(PROTO_CHIP_ERASE);
                   send(PROTO_EOC);
-                  return get_sync(10000);
+                  return get_sync(20000);

Collaborator

andyp1per Jan 29, 2025

Is this a timeout? Maybe add a comment

Member Author

bugobliterator Jan 30, 2025

done

libraries/AP_IOMCU/iofirmware/analog.cpp Outdated Show resolved Hide resolved

libraries/AP_IOMCU/iofirmware/iofirmware.cpp Outdated

-                                   STM32_DMA_CR_MINC | STM32_DMA_CR_TCIE);
+                                   STM32_DMA_CR_MINC | STM32_DMA_CR_TCIE
+              #if defined(STM32H7)
+                                   | (1<<20)

Collaborator

andyp1per Jan 29, 2025

needs a define

Member Author

bugobliterator Jan 30, 2025

I will leave a comment, we don't have a standard define for this as this is from an errata

Member Author

bugobliterator Jan 30, 2025

fixed through define and also in UARTDriver

libraries/AP_IOMCU/iofirmware/iofirmware.cpp Outdated

-                                   STM32_DMA_CR_MINC | STM32_DMA_CR_TCIE);
+                                   STM32_DMA_CR_MINC | STM32_DMA_CR_TCIE
+              #if defined(STM32H7)
+                                   | (1<<20)

Collaborator

andyp1per Jan 29, 2025

ditto

Member Author

bugobliterator Jan 30, 2025

fixed

libraries/AP_IOMCU/iofirmware/iofirmware.cpp

-                      uart->usart->CR1 = cr1 | USART_CR1_SBK;
+                      CLEAR_ISR_FLAG(LBD);
+                      SEND_BREAK();

Collaborator

andyp1per Jan 29, 2025

this is a slight change in behaviour because this was a local variable for threading reasons. Ideally would preserve the current semantics

Member Author

bugobliterator Jan 30, 2025

I am going to change it back to original as it doesn't really matter in this context.

But the change is actually correct. Firstly a thread cannot interrupt an interrupt, another higher priority interrupt can. Secondly, only bit we want to set in CR1 is Line Break, rest of the bits we don't want to touch. With the older method there's a chance we can end up doing just that.
Basically if there was another interrupt or peripheral event (between where we store cr1 and call SEND_BREAK) where the CR1 is changed we will end up changing it again unknowingly. Fortuntely we don't mess with CR1 outside of init and this interrupt, so it is a non-issue. But in general this can cause issue.

libraries/AP_IOMCU/iofirmware/iofirmware.cpp

@@ @@ -176,7 +257,7 @@ static void idle_rx_handler(hal_uart_driver *uart) @@
                   if ((sr & USART_SR_TC) && (cr1 & USART_CR1_TCIE)) {
                       /* TC interrupt cleared and disabled.*/
-                      uart->usart->SR &= ~USART_SR_TC;
+                      CLEAR_ISR_FLAG(TC);

Collaborator

andyp1per Jan 29, 2025

Might be better to use the MODIFY_REG() macro that you used in bdshot

Member Author

bugobliterator Jan 29, 2025

That doesn't work, here. Its a completely different operation for STM32H7. its a modify register operation in STM32F1 but a simple write operation in H7. So this is the only way.

Collaborator

andyp1per Jan 30, 2025

ok

libraries/AP_IOMCU/iofirmware/iofirmware.cpp Outdated

    
                      uart->dmatx = dmaStreamAllocI(STM32_UART_USART2_TX_DMA_STREAM,

                                                      STM32_UART_USART2_IRQ_PRIORITY,

                      uart->dmatx = dmaStreamAllocI(HAL_IO_FMU_COMMS_TX_DMA_STREAM,

                                                      12,  //IRQ Priority

Collaborator

andyp1per Jan 29, 2025

This is bypassing the config, thing it should use the define or put a define above

Member Author

bugobliterator Jan 30, 2025

yeah, I was trying to avoid adding yet another define that's going to remain untouched. Will add this to define as well.

Contributor

tridge Feb 5, 2025

still pending?

libraries/AP_IOMCU/iofirmware/iofirmware.cpp

@@ @@ -447,7 +567,7 @@ void AP_IOMCU_FW::update() @@
                   loop_counter++;
                   if (do_reboot && (last_ms > reboot_time)) {
-                      hal.scheduler->reboot(true);
+                      hal.scheduler->reboot(false);

Collaborator

andyp1per Jan 29, 2025

why?

Member Author

bugobliterator Jan 30, 2025

This was incorrect to begin with. We don't actually want to hold in bootloader here. And we don't for iomcu bootloader we are currently using (as its extremely old and didn't even have that feature).

The behaviour of this in current bootloader is that it holds the MCU in bootloader forever. If you reboot ardupilot, the iomcu will sit in bootloader forever, hence failing IOMCU sanity check and force trigger a firmware update everytime.

Collaborator

andyp1per Jan 30, 2025

ok sounds reasonable. Probably needs testing on other boards that have iofirmware update issues - particularly Pixhawk6X


          modules: update ChibiOS

2c43289

bugobliterator force-pushed the pr-cubered-iomcu branch 2 times, most recently from 5fad99f to 18c7ecc Compare

January 30, 2025 02:56

bugobliterator requested a review from andyp1per

January 30, 2025 03:03

bugobliterator force-pushed the pr-cubered-iomcu branch from 18c7ecc to d8a852b Compare

January 30, 2025 03:32

andyp1per added the NeedsTesting label

andyp1per approved these changes

View reviewed changes

Collaborator

andyp1per left a comment

LGTM, just needs testing iofirmware upgrades on some of the other boards (e.g. Pixhawk6X)

tridge requested changes

View reviewed changes

Contributor

tridge left a comment

I'm glad to see this becoming an option, just need to work out how to properly dynamically choose if UART is IOMCU or some other protocol, the method in this PR doesn't look right
also, this does change the size of iofirmware_f103_lowpolh.bin by 8 bytes, need to work out why

libraries/AP_HAL_ChibiOS/UARTDriver.cpp Outdated

@@ @@ -1884,6 +1884,12 @@ void usb_initialise(void) @@
               // disable TX/RX pins for unusued uart
               void UARTDriver::disable_rxtx(void) const
               {
+              #if HAL_WITH_IO_MCU
+                  if (sdef.get_index() == HAL_UART_IOMCU_IDX) {

Contributor

tridge Jan 30, 2025

why is this added? it would be valid to call this function on IOMCU uart if BRD_IO_ENABLE was false for example. We don't do that now, but I don't understand the special case
I suspect this is all about the serial port being able to be either a link to IOMCU or a normal serial manager uart (eg. for PPP of mavlink)
why not add a SERIALn_PROTOCOL=IOMCU enum value, then handle this in SerialManager?

libraries/AP_IOMCU/iofirmware/analog.cpp Show resolved Hide resolved

Tools/AP_Bootloader/AP_Bootloader.cpp Outdated

@@ @@ -87,6 +87,12 @@ int main(void) @@
                   bool try_boot = false;
                   uint32_t timeout = HAL_BOOTLOADER_TIMEOUT;
+              #if defined(HAL_BL_IOMCU_FW_DETECT) && HAL_BL_IOMCU_FW_DETECT
+                  if (is_iomcu_fw_present()) {

Contributor

tridge Jan 30, 2025

why is this needed? This seems to be forcing a shorter timeout if there is already a IO fw present, which seems odd

Member Author

bugobliterator Jan 30, 2025

Current iofirmware bootloader has 200ms timeout. Also if iofirmware doesn't boot faster than when AP_IOMCU tries to detect it, it will simply try to reflash.

Tools/AP_Bootloader/support.cpp Outdated Show resolved Hide resolved

libraries/AP_HAL_ChibiOS/Scheduler.cpp Outdated

+              void AP_memory_guard_error(uint16_t error);
+              void AP_memory_guard_error(uint16_t error) {
+                  // simply panic for now
+                  AP_HAL::panic("PANIC: memory guard error %u", error);

Contributor

tridge Jan 30, 2025

wouldn't a reboot be better? a panic will loop forever, causing the vehicle to crash with no IO

libraries/AP_HAL_ChibiOS/hwdef/scripts/chibios_hwdef.py

+                          self.env_vars['IOMCU_FW'] = 0
+                      # check if heater pin defined
+                      if 'HEATER' in self.bylabel.keys():

Contributor

tridge Jan 30, 2025

we already handle not having a heater with the HEATER_SET() macro, why also handle it at the build level?

Member Author

bugobliterator Jan 31, 2025

This is needed to disable multiple builds for highpol and lowpol heaters. With hwdef without Heaters we were still doing that.

libraries/AP_HAL_ChibiOS/hwdef/scripts/chibios_hwdef.py Outdated Show resolved Hide resolved

libraries/AP_HAL_ChibiOS/hwdef/scripts/chibios_hwdef.py Outdated

-                              '#define HAL_UART_IO_DRIVER ChibiOS::UARTDriver uart_io(HAL_UART_IOMCU_IDX)\n'
-                          )
-                          serial_list.append(self.config['IOMCU_UART'][0])
+                          if self.config['IOMCU_UART'][0].startswith('HAL_SERIAL'):

Contributor

tridge Jan 30, 2025

if we made AP_IOMCU take a pointer to a AP_HAL::UARTDriver instead of a reference then we could have an AP::iomcu()->set_uart() call that could be made from AP_SerialManager. That would enable runtime configuration.

Member Author

bugobliterator Jan 31, 2025

I think I am going to do this in a separate PR, as that will require quite a decent amount of change to AP_IOMCU. A separate PR for this change will be much easier to review.

For this PR I can go part of the way and use SerialProtocol and hal.serial instead of current method of declairing a separate serial driver for all iomcu users.

libraries/AP_Vehicle/AP_Vehicle.cpp Outdated

+                  if (BoardConfig.io_enabled()) {
+                      // If IO is enabled, we need to set the protocol to None
+                      // as the IO MCU UART is handled by the AP_IOMCU class
+                      serial_manager.set_protocol_and_baud(HAL_UART_IOMCU_IDX, AP_SerialManager::SerialProtocol_None, 0);

Contributor

tridge Jan 30, 2025

SerialProtocol_IOMCU would make more sense

Member Author

bugobliterator Jan 31, 2025

changed to SerialProtocol_IOMCU

libraries/AP_Vehicle/AP_Vehicle.cpp Outdated

+                      // as the IO MCU UART is handled by the AP_IOMCU class
+                      serial_manager.set_protocol_and_baud(HAL_UART_IOMCU_IDX, AP_SerialManager::SerialProtocol_None, 0);
+                  } else {
+                      iomcu.wipe();

Contributor

tridge Jan 30, 2025

why are we wiping the fw if IO not enabled?

Member Author

bugobliterator Jan 30, 2025

This is to allow flashing a different firmware to secondary. We have to reduce the boot time if IOMCU is enabled to 200ms, which is not long enough for it to be caught over USB. To allow updating Secondary MCU over USB again we need wipe iofirmware.

Member Author

bugobliterator Jan 31, 2025

Not relevant anymore, moved to different procedure to allow updates other than iofirmware.

bugobliterator force-pushed the pr-cubered-iomcu branch from d8a852b to 8d6f33c Compare

January 31, 2025 03:51

Member Author

bugobliterator commented Jan 31, 2025

@tridge I have pushed changes per your comments.

got rid of the bootloader changes, and using bootloader CRC to verify IOMCU at boot.
Added option SerialProtocol_IOMCU, currently serialmanager is not doing anything with it.
moved to using serialxDriver for IOMCU as well.

bugobliterator force-pushed the pr-cubered-iomcu branch from 8d6f33c to 23e09cd Compare

January 31, 2025 03:56

bugobliterator added 6 commits

January 31, 2025 15:18


          AP_HAL_ChibiOS: separate method for memory error on IOMCU FW

f1ddfcf


          AP_HAL_ChibiOS: add support for IOMCU on Secondary

4d1722f


          AP_HAL_ChibiOS: add support for using hal serial as IOMCU Serial

10f97de


          AP_IOMCU: add support for new version of Bootloader

38272df


          AP_IOMCU: add support for custom uart options and iomcu flash size

6134d07


          AP_IOMCU: add support for running IOFW on CubeRedSecondary

4222d19

bugobliterator added 11 commits

January 31, 2025 15:18


          AP_HAL_ChibiOS: add CubeRedSecondary-IO hwdef.dat

e2a939c


          AP_HAL_ChibiOS: fix CubeRedPrimary-PPPGW build

c811808


          hwdef: add env variable to set IOMCU Heater feature

3eb1b6f


          AP_IOMCU: remove heater polarity setting for IOMCU without heater

1b06686


          scripts: add cuberedsecondary-io build to IOFirmware build script

bad025d


          AP_HAL_ChibiOS: use TRBUFF define instead of hardcoding

40238bd


          AP_Vehicle: set iomcu serialmanager protocol if iomcu is enabled

5e287a6


          AP_IOMCU: ignore call to shutdown if iomcu not initialised

c18910b

fixes watchdog trigger on call to trigger after enabling iomcu from disabled state


          AP_SerialManager: add support for SerialProtocol_IOMCU

78effbf


          AP_OSD: add option for IOMCU SerialProtocol

86414c1


          IO_Firmware: add iofirmware_cubered

d67d2ce

bugobliterator force-pushed the pr-cubered-iomcu branch from 23e09cd to d67d2ce Compare

January 31, 2025 04:18


          AP_HAL_ChibiOS: put tables for declination in external flash

3ca947f

bugobliterator added the DevCallEU label

peterbarker removed the DevCallEU label

peterbarker requested changes

View reviewed changes

Tools/AP_Bootloader/AP_Bootloader.cpp Outdated Show resolved Hide resolved

libraries/AP_HAL_ChibiOS/hwdef/CubeRedSecondary-IO/hwdef.dat

+              define HAL_USE_RTC FALSE
+              define HAL_NO_UARTDRIVER TRUE
+              define HAL_LOGGING_ENABLED 0

Contributor

peterbarker Feb 5, 2025

Why are these in here? These are the default for io firmware... (defaults_periph.h)

libraries/AP_HAL_ChibiOS/hwdef/CubeRedSecondary-IO/hwdef.dat

		define HAL_GPIO_PIN_SPEKTRUM_OUT PAL_LINE(GPIOC,11U)


		define AP_NETWORKING_BACKEND_PPP 1

Contributor

peterbarker Feb 5, 2025

This is odd?

libraries/AP_IOMCU/iofirmware/rc.cpp

Comment on lines +65 to +68

		#define STR(x) XSTR(x)
		#define XSTR(x) #x

Contributor

peterbarker Feb 5, 2025

Unused?

peterbarker reviewed

View reviewed changes

libraries/AP_HAL_ChibiOS/Scheduler.cpp

@@ @@ -95,6 +95,14 @@ THD_WORKING_AREA(_monitor_thread_wa, MONITOR_THD_WA_SIZE); @@
               #define AP_HAL_CHIBIOS_IN_EXPECTED_DELAY_WHEN_NOT_INITIALISED 1
               #endif
+              #if defined(STM32H7) && defined(IOMCU_FW)

Contributor

peterbarker Feb 5, 2025

Suggested change

      
            #if defined(STM32H7) && defined(IOMCU_FW)
          
            #if AP_REBOOT_ON_MEMGUARD_ERROR

... and default it off and define to true in the hwdef.

tridge requested changes

View reviewed changes

Contributor

tridge left a comment

i'd like to see binary cmp showing no change in IO fw builds

libraries/AP_IOMCU/fw_uploader.cpp

@@ @@ -97,6 +101,14 @@ bool AP_IOMCU::upload_fw(void) @@
                   }
                   debug("found bootloader revision: %u", unsigned(bl_rev));
+                  if (bl_rev > 2) {
+                      // verify the CRC of the IO firmware
+                      if (verify_rev3(fw_size)) {

Contributor

tridge Feb 5, 2025

why add this in? It makes me a bit nervous to be changing this behaviour for existing boards
maybe ifdef it for only this "modern" IOMCU

libraries/AP_IOMCU/fw_uploader.cpp

-                      debug("Err: failed to contact bootloader");
+                  uint32_t bl_rev, unused;
+                  if (!get_info(INFO_BL_REV, bl_rev) ||
+                      !get_info(INFO_BOARD_ID, unused) ||

Contributor

tridge Feb 5, 2025

I think make these changes under #ifdef for IOMCU_CHIBIOS_BOOTLOADER

libraries/AP_IOMCU/fw_uploader.cpp

@@ @@ -281,7 +293,7 @@ bool AP_IOMCU::erase() @@
                   debug("erase...");
                   send(PROTO_CHIP_ERASE);
                   send(PROTO_EOC);
-                  return get_sync(10000);
+                  return get_sync(20000); // 20s timeout for erase

Contributor

tridge Feb 5, 2025

ifdef IOMCU_CHIBIOS_BOOTLOADER or define for timeout

libraries/AP_IOMCU/fw_uploader.cpp

@@ @@ -417,12 +429,13 @@ bool AP_IOMCU::verify_rev3(uint32_t fw_size_local) @@
                   /* compare the CRC sum from the IO with the one calculated */
                   if (sum != crc) {
                       debug("CRC wrong: received: 0x%x, expected: 0x%x", (unsigned)crc, (unsigned)sum);
+                      get_sync();

Contributor

tridge Feb 5, 2025

ifdef

libraries/AP_IOMCU/fw_uploader.cpp

                       return false;
                   }
                   crc_is_ok = true;
-                  return true;
+                  return get_sync();

Contributor

tridge Feb 5, 2025

ifdef

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels