Simplify Malyan variants

matthijskooijman commented 4 years ago

Today, I noticed some startup customizations in the Malyan M200 and Mx00 variants, of which I wonder if they are really needed. The way they work now, might pose complications for implementing a custom reset handler for #710, so I wonder if these customizations cannot be removed, or generalized.

There's two things here:

It remaps the 0x0 address block to RAM rather than flash: https://github.com/stm32duino/Arduino_Core_STM32/blob/213a3d5d3781fc5e46a47103bef4a8e5e9b723fa/variants/MALYANMx00_F070CB/variant.cpp#L101-L108 (which requires moving the interrupt table to flash, and also requires some linker script changes to facilitate this).
It has a custom startup assembly code.

Looking at the latter, it seems the changes compared to the default scripts are minimal. It seems there used be an "enable fan" command in there, but it is commented out. The actual changes do noot seem so relevant: disabling/enabling interrupts (cpsid / cpsie), some barriers (dsb / isb) and a breakpoint (bkpt) in an unreachable place. The M200 startup actually adds an initialization of the stack pointer (which should not actually be needed, since a reset already does that), but looking at other default startup code all these load the stack pointer, except for all of the F1 startup code. So I doubt any of these changes are intentional, they might just be the result o basing off an older or newer version of the startup scripts.

Here's the startup code diffs (with comment and whitespace changes removed):

$ diff variants/MALYANM200_F103CB/startup_M200_f103xb.S system/Drivers/CMSIS/Device/ST/STM
32F1xx/Source/Templates/gcc/startup_stm32f103xb.s  -u
@@ -77,14 +60,6 @@
   .weak Reset_Handler
   .type Reset_Handler, %function
 Reset_Handler:
-/* Initialize the stack pointer and shut down bootloader interrupts */
-  /* bl turnOnFan */
-  ldr   r0, =_estack
-  mov   sp, r0          /* set stack pointer */
-  cpsid i
-  dsb
-  isb
-  cpsid f

 /* Copy the data segment initializers from flash to SRAM */
   movs r1, #0
@@ -116,11 +91,6 @@

 /* Call the clock system intitialization function.*/
     bl  SystemInit
-
-/* Re-enable handlers */
-cpsie i
-cpsie f
-
 /* Call static constructors */
     bl __libc_init_array
 /* Call the application's entry point.*/
@@ -140,7 +110,6 @@
 Default_Handler:
 Infinite_Loop:
   b Infinite_Loop
-  BKPT #0
   .size Default_Handler, .-Default_Handler
 /******************************************************************************
 *

$ diff variants/MALYANMx00_F070CB/startup_Mx00_f070xb.S system/Drivers/CMSIS/Device/ST/STM32F0xx/Source/Templates/gcc/startup_stm32f070xb.s 
@@ -62,13 +49,9 @@
   .weak Reset_Handler
   .type Reset_Handler, %function
 Reset_Handler:
-/*   bl manualFan */
   ldr   r0, =_estack
   mov   sp, r0          /* set stack pointer */

-  cpsid i
-  cpsid f
-
 /* Copy the data segment initializers from flash to SRAM */
   ldr r0, =_sdata
   ldr r1, =_edata
@@ -85,7 +68,7 @@
   adds r4, r0, r3
   cmp r4, r1
   bcc CopyDataInit
-
+  
 /* Zero fill the bss segment. */
   ldr r2, =_sbss
   ldr r4, =_ebss
@@ -102,10 +85,6 @@

 /* Call the clock system intitialization function.*/
   bl  SystemInit
-
-  cpsie i
-  cpsie f
-
 /* Call static constructors */
   bl __libc_init_array
 /* Call the application's entry point.*/

Looking at the history, relevant PRs are #382, #422 and #354. The last one mentions "the startup assembler must account for some oddities of the bootloader which is already running". It suspect, this means that there is a bootloader in regular flash, which lives in the first 8K, so the linker script is modified to put the sketch after the bootloader. Since the ISR table lives at 0x0, the 0x0 block must be remapped to RAM, and the ISR vector table copied there. Finally, because maybe the bootloader neglects to initialize the SP (as the hardware normally does), the reset handler must do this.

@xC0000005, can you confirm or shed more light here? In particular, how much of this applies to which variant? It seems Mx00 remaps memory and copies the ISR table, but M200 does not (but it does leave 8K of memory free?). Is the bootloader different and does that maybe handle remapping already?

matthijskooijman commented 4 years ago

Oh, I just remembered that F1 cannot actually do remapping, so I suspect that the M200 bootloader which runs on F1, maybe copies the ISR table into the start of flash when uploading the sketch. Are any of these bootloaders open-source, or is it all closed?

xC0000005 commented 4 years ago

You're close but off just a bit. First, a bit of history and some explanations - I bought one of these printer off of amazon used, loved it, and got frustrated with the software, which is closed source. Instead of doing the sane thing and moving to ANY other board, I ordered a few bluepills off the internet, downloaded ST's 103 programming guide, and decided to reverse engineer the hardware and software. So these files have the remnants of some terrible, terrible hacks in them, but most are needed in some form.

Malyan's bootloader is not open source, but if you want to mail me at at dot , I have extracted it (and figured out their stupid scheme that prevents people from replacing the chip when it burns out).

The bootloader is the first 8k or so, and it has some peculiarities we're working around in the variants. On the M0 070 machines, there's no VTOR (unlike the 103), so the standard way of having your own ISRs is to copy the table to ram and set the remap bit, which is exactly what that variant does. That code should not be removed, or you'll stop it working completely. Use a nucleo 070 to prove this to yourself - it's what I did.

Now, what about the rest of the ugly hacks? CPSI D,F - that's me hacking my way into figuring things out, by way of turnOnFan. These boards don't have SWD pads exposed, so my only way to figure things out in the machine at first was to have a function which turned on the fans if I reached a stage of intialization and then looped forever (until the watchdog killed the machine).

Thus, by repeatedly testing different assertions and seeing if the fan still turned on, I was able to figure out where in the boot sequence things failed.

Debuggging via fan. I also have (somewhere) a version that bit-bangs data over the fan port, using four different speeds to do 4 bit encoding of data, but that's another story.

Loading stack pointer - sure seems like something the bootloader would do, right? The bare metal 103 does it, so the bootloader should. It does not, so don't remove that line or once again, you'll break everything.

CPI...Here's the only thing I'm moderately ashamed of. Via turnOnFan, I knew we were reaching my ResetHandler. I knew I could take control of the hardware, and I also knew we were crashing into a fault handler during static variable initialization. What I did not know at the time, but understand now, is that manufacturers like Malyan (and Lerdge, and Chitu) use a n LD script which pokes a few variables into fixed positions in RAM. These positions are not necessarily contiguous and serve as another form of copy protection, in addition to Lerdge "encrypting" and malyan setting read protection.

Thanks to @clearchris's work on Klipper, we now know that what is really going on is that the Malyan bootloader (which runs on their M100, M200, M300, M350 machines) leaves the systick handler running when it starts the firmware, and watches some data in RAM. During init, depending on what gets copied where, it can crash as it acts on data that is NOT what it expects.

I had no idea about the systick at the time, but I did know that if I disabled the handlers during init, I could get past it, and once the system was intialized, everything ran fine. So I did, and proceeded to go on with working on Marlin on STM32.

It's ok to remove the turnOnFan reference. You're welcome to convert the cpsd i,f to turning off systick. Don't remove the SP changes, or I'll have to put them back. Don't remove the remapping of the NVIC for the same reason.

xC0000005 commented 4 years ago

Github ate my attempt to obfuscate the email address, but it's my handle at gmail dot com.

matthijskooijman commented 4 years ago

Thanks for your elaborate reply, that clarifies a lot and gives good boundaries for cleaning up. Kudos for figuring all this stuff out :-)

One more question, though: How is an upload triggered? Does the bootloader always wait at startup for a bit, or is there some "update" button to trigger it? Can the sketch also reboot into the bootloader somehow? And if no sketch is uploaded, does the bootloader then also leave systick running or does it do quick jump to the sketch?

xC0000005 commented 4 years ago

The only way to update firmware (for people without soldering skills) is to update via SD card - the bootloader’s main purpose is to look at the SD card for a file (firmware.bin) and a flag file (fcupdate.flg) and if both are present, firmware.bin will be flashed onto the board. The bootloader also implements an ESP8266 OTA update protocol which it can perform. The sketch can reboot into the bootloader via a fixed address call, and if no sketch is loaded, the bootloader crashes hard.

On Mar 20, 2020, at 1:34 PM, Matthijs Kooijman notifications@github.com wrote:

Thanks for your elaborate reply, that clarifies a lot and gives good boundaries for cleaning up. Kudos for figuring all this stuff out :-)

One more question, though: How is an upload triggered? Does the bootloader always wait at startup for a bit, or is there some "update" button to trigger it? Can the sketch also reboot into the bootloader somehow? And if no sketch is uploaded, does the bootloader then also leave systick running or does it do quick jump to the sketch?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stm32duino/Arduino_Core_STM32/issues/1000#issuecomment-601898901, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVGS4JHIQPR3IABGMG7W5LRIPHN5ANCNFSM4LQKWACA.

matthijskooijman commented 4 years ago

So that sounds like the systick is always started, even if no update is performed? IOW, doing another reset and expect the bootloader to start the sketch immediately without enabling systick is not going to work (unless the bootloader maybe skips all its work and directly boots the sketch depending on the reset reason flag, that's what optiboot on AVR does IIRC).

xC0000005 commented 4 years ago

The systick is always started. In fact, the only time things work the way you’d expect (system is bare/uninitialized) is when a flash update has been performed. Like I said, we could turn off Systick instead of disabling fault/interrupts, but since the current code works, I’ve always had other things to do (like figure out how to get DMA to work in the core).

On Mar 20, 2020, at 1:48 PM, Matthijs Kooijman notifications@github.com wrote:

So that sounds like the systick is always started, even if no update is performed? IOW, doing another reset and expect the bootloader to start the sketch immediately without enabling systick is not going to work (unless the bootloader maybe skips all its work and directly boots the sketch depending on the reset reason flag, that's what optiboot on AVR does IIRC).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stm32duino/Arduino_Core_STM32/issues/1000#issuecomment-601904127, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHVGS4ITLSOSMYKRCSCYOSDRIPJATANCNFSM4LQKWACA.

clearchris commented 4 years ago

Yep, I can confirm everything JC has said. 100% correct.

matthijskooijman commented 4 years ago

I thought a bit more on how to generalize this, my thoughts are below (mostly for my own reference, but feedback is welcome of course).

SP loading

It would be nice if the default startup script would just always load SP that. I'm not quite sure why this is done for all families except for F1 now. I checked the latest version (1.8.0) of the F1 firmware package, which also does not include any SP initialization code (but this is the same version as included in this repo, according to 332dde3a47). I also checked the latest version (1.25.0) of the F4 firmware package, which does still contain the SP load, so it is not something that was global disabled.

I think it would be good to use the Reset_Handler override trick I made for #710 to provide a generic (not-board-specific) Reset_Handler that (for all F1 chips) starts by loading SP. Probably not needed in most cases, but should not hurt either.

Interrupt / systick disabling

Currently, interrupts are disabled during memory initialization to prevent the bootloader from interfering. If this just hapens through the systick handler, I think it would be better to disable that, since that just requires running some extra code at the start of the reset handler (no need to re-enable interrupts at the end).

Also, I think that with the current code, there might even be a race condition. Interrupts are re-enabled after calling SystemInit, which only resets system clocks and does not touch systick. Then, static constructors are called (through __libc_init_array which I could not find just now, but I think it refers to the .init_array section in the linker script, which is filled by constructors in the .initx sections). One such constructor is premain(), which calls init(), which calls hw_config_init which calls HAL_Init() which inits systick. All the while, the systick ISR could trigger and still run the bootloader. This could be fixed by just disabling the systick in early startup, which should be safe until it is re-enabled. This could probably be done unconditionally, or maybe triggered by a define in the variant.

ISR table remappping

This also needs to be kept, but we could maybe also integrate this remapping into the core and trigger it with a variant define?

This remapping should probably also be done earlier: Mx00 now does it in initVariant(), which happens at the start of main(), but by then some interrupts (systick, USB) might already have been enabled so these could still call into the bootloader, I believe (this is a variant of the above described race condition).

Linker scripts

The M200 linker script only has an offset in the flash (compared to a cleanly generated script using CubeMX). We could probably unhardcode this and use the LD_FLASH_OFFSET approach used by some other linker scripts (or maybe even generalize that with the linker override script, but I'll have to test that).

The Mx00 linker script additionally defines a VTRAM section for the vector table, that should also be kept (but might also be generalized with the linker override script, if we can somehow figure out how big it should be).

fpistm commented 4 years ago

Is there any update on this ? Thanks in advance.

matthijskooijman commented 4 years ago

I haven't had time to work on this anymore since my last comment, unfortunately.

fpistm commented 4 years ago

Thanks for the quick answer. I guess I will have to get in mind this issue for #710

matthijskooijman commented 3 years ago

I had another look at this today, because I have a board that needs ISR table remapping, so I looked at how these variants did that. I haven't done any work here, but I've seen that with #1091 merged, most of the above simplifications (and my previous summary comment https://github.com/stm32duino/Arduino_Core_STM32/issues/1000#issuecomment-603805335) are still applicable, except that the flash offset in the linker script is no longer hardcoded but uses LD_FLASH_OFFSET now. This means that M200 no longer has a custom linker script, and Mx00 has one but just for the VTRAM stuff.

stm32duino / Arduino_Core_STM32