kcuzner / led-watch

A miniature ARM-powered LED wristwatch
http://kevincuzner.com/2017/04/18/the-led-wristwatch-a-more-or-less-completed-project/
MIT License
95 stars 16 forks source link

ARM assembly error in file /common/src/debug.c line 55 #4

Closed Eqqm4n closed 1 year ago

Eqqm4n commented 1 year ago

Hello- been porting this code and compiling under Segger Embedded Studio for ARM 7.12 (I care about the bootloading, not the watch functionality, as a point of reference). This generates a compile error in debug.c line 55, the line itself is 'movs r1, lr'. The error given is:

invalid instruction, any one of the following would fix this: instantiated into assembly here operand must be an immediate in the range [0,255] instantiated into assembly here operand must be a register in range [r0, r7] instantiated into assembly here

Since I noticed that the original source looked to be compiled as a project for Atollic Studio which is now part of ST's CubeMX IDE, I compiled this under Cube as well and happened to get this same error. This code is included as part of the replacement Hard Fault handler, so I looked at the default handler provided by Segger and saw it was virtually identical. So replacing the 'movs' with 'mov' as Segger uses addressed the problem. Note that this doesn't ensure the code is functionally correct, it just removes a compilation error. The complete section is as follows: movs r0, #4 movs r1, lr, test r0, r1 I believe the 's' suffix tells the ALU status bits to update after the operation. So if the test command is checking these ALU bits based on the move from the link register, then this will be incorrect, but I don't know how to rewrite this to remove the compilation error and still get the same result of updating the status bits. It could also be that the test command does not rely on the status bits from the previous two instructions, in which case they can both be replaced with 'mov' (Segger has 'movs' then 'mov') but I haven't done a thorough investigation of what is happening to the processor in this sequence.

kcuzner commented 1 year ago

I've seen this happen when the wrong core is selected. This specifically was written to compile for the STML052, which I believe is a Cortex-M0+ (and the + is a big deal). The other thing is that this was originally written for GCC, via arm-none-eabi-gcc. The original source of cmsis was indeed found in STM32Cube, but the rest of it was written by myself and did not use any IDE (just Makefiles and GCC). That said, looking at debug.c it doesn't fit with my own style and so I wouldn't be surprised if I lifted that from somewhere else. But nonetheless, it was known to compile for GCC.

NOTE: Edited for accuracy

It's been a long time, but I believe what that code is doing is checking if it should be using the PSP or MSP as the stack pointer. The stack pointer is then back-tracked by 20 bytes and that value used for the HardFault_HandlerC's array argument. The hardware pushes that context onto the stack when a hard fault occurs and so if we figure out which stack pointer was being used (which I think is encoded in lr in the 2nd bit (0th bit being the LSB)) we can load the context for the function that was being executed which led to the hard fault being executed. I recall that on GDB I was able to have it actually decode the full stack once this information was given. But I'm guessing. It's been a long time like I said :).

Looking at your code, I don't think there's any reason we need to have mov update the status flags. All it's trying to do is LR & 0x4 and the tst operation is setting the flags that the subsequent beq uses. So I think that change is safe.

kcuzner commented 1 year ago

I would suggest if you're hoping to use my bootloader that you consult my https://github.com/kcuzner/midi-fader project for the latest iteration. This one hasn't been tested in a while and I do recall some unreliability issues I had. The midi-fader one is better.

Eqqm4n commented 1 year ago

Thank you for the response. All of the STM32L0 line should be Cortex M0+so I wouldn't expect a compilation difference on those lines between the L052 and L062. The code seems questionable on its face since with two 'movss' commands in a row the flags from the first move are being replaced without even being tested. I'm sure it probably hasn't been properly reviewed in decades :).

I'll give the midi-fader project a check. If you actively maintain projects it may be worth it to split bootloading off into its own separate repository. I only ended up here after finding a reference to your project on an Adafruit blog when looking up STM32 bootloaders on the web.

kcuzner commented 1 year ago

I'll take that suggestion for having a separate repository for the bootloader, though my personal projects (which all of these are) tend to focus more on learning something new and thus I tend to go with the "copy paste" style of leverage, iterating on the thing after pasting and rarely revisiting or backporting.