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

+1,394 −863

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

@@ @@ -58,6 +58,14 @@ @@
               extern AP_IOMCU iomcu;
               #endif
+              #ifdef IOMCU_FW

Collaborator

andyp1per Jan 29, 2025

This is only with H7 I think? Probably should add that to the define.

Member Author

bugobliterator Jan 29, 2025 •

edited

Loading

done

libraries/AP_IOMCU/AP_IOMCU.cpp Outdated

@@ @@ -100,6 +104,34 @@ void AP_IOMCU::init(void) @@
                   initialised = true;
               }
+              /*
+                disable the IOMCU operation by erasing iofirmware

Collaborator

andyp1per Jan 29, 2025

I think this should have a different name given the fairly radical function of its operation

Member Author

bugobliterator Jan 30, 2025

thoughts on name, erase is already taken.

Member Author

bugobliterator Jan 30, 2025

changing it to wipe

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.

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

bugobliterator added 6 commits

January 30, 2025 13:45


          modules: update ChibiOS

2c43289


          AP_Bootloader: add support for detecting and reducing timeout if IOMC…

898834b

…U FW is detected


          AP_HAL_ChibiOS: separate method for memory error on IOMCU FW

63aa818


          AP_HAL_ChibiOS: add support for IOMCU on Secondary

46e2e0f


          AP_HAL_ChibiOS: set detect IOMCU fw on secondary and set boot time ac…

625cbcb

…cordingly


          AP_HAL_ChibiOS: add support for using hal serial as IOMCU Serial

6850d15

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 added 8 commits

January 30, 2025 14:32


          AP_IOMCU: add support for new version of Bootloader

d48dfd4


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

61fbad3


          AP_IOMCU: add support for running IOFW on CubeRedSecondary

1ec448a


          AP_HAL_ChibiOS: add CubeRedSecondary-IO hwdef.dat

1bee97f


          AP_HAL_ChibiOS: fix CubeRedPrimary-PPPGW build

02beaa0


          bootloaders: update CubeRedSecondary bootloader

5d0b44c


          hwdef: add env variable to set IOMCU Heater feature

d367c00


          AP_IOMCU: remove heater polarity setting for IOMCU without heater

0806caf

bugobliterator added 5 commits

January 30, 2025 14:32


          scripts: add cuberedsecondary-io build to IOFirmware build script

5c0bbee


          AP_IOMCU: add option to remove iofirmware if disabled

30880c8


          IO_Firmware: add iofirmware_cubered

b8e3565


          AP_HAL_ChibiOS: use TRBUFF define instead of hardcoding

027a364


          AP_Vehicle: wipe iofirmware before serialmanager is iomcu is disabled

d8a852b

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

@@ @@ -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

@@ @@ -22,6 +22,8 @@ @@
               #include "hal.h"
               #include "analog.h"
+              #ifdef HAL_DISABLE_ADC_DRIVER

Contributor

tridge Jan 30, 2025

is this supposed to be ifndef?

Member Author

bugobliterator Jan 30, 2025

no, that is correct. If AP::HAL ADC Driver is disabled analog.cpp handles ADC periph

Otherwise we use hal.analogin

Tools/AP_Bootloader/AP_Bootloader.cpp

@@ @@ -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

    
               */

              #if defined(HAL_BL_IOMCU_FW_DETECT) && HAL_BL_IOMCU_FW_DETECT

              #define HAL_IOMCU_MAGIC 0x937E638A

              uint32_t is_iomcu_fw_present(void)

Contributor

tridge Jan 30, 2025

i'd like to understand why this is needed

Member Author

bugobliterator Jan 30, 2025

This is so we can reduce the boot time if IOMCU is present to 200ms,. This is done by reading the IOMCU_FW_DETECT magic written to the end of flash.

I think I am going to add a boot time field to AP_CheckFirmware instead. Was looking for a shortcut but this hasn't turned out to be a clean implementation.

libraries/AP_HAL_ChibiOS/Scheduler.cpp

+              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?

libraries/AP_HAL_ChibiOS/hwdef/scripts/chibios_hwdef.py

@@ @@ -1052,6 +1052,17 @@ def write_mcu_config(self, f): @@
                       else:
                           self.env_vars['IOMCU_FW'] = 0
+                      if self.get_config('IOMCU_FW', required=False):

Contributor

tridge Jan 30, 2025

cut and paste duplication?

libraries/AP_HAL_ChibiOS/hwdef/scripts/chibios_hwdef.py

-                              '#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

+                  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

libraries/AP_Vehicle/AP_Vehicle.cpp

+                      // 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.

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

Labels