fix compilation warnings #1516
fix compilation warnings #1516
Conversation
There was a problem hiding this comment.
It looks to me like at least some of these warnings are being caused by missing compiler arguments in your build system, rather than problems in the code. See specific comments.
The missing void changes are valid though, as is removing the erroneous argument to usb_controller_run.
|
Thanks for your review. |
|
Okay. We are going to try to fix the LPC43XX_M4 define on our side. Let me commit the removal. |
|
Thanks to your support / review, we are now trying to fix our build system X-D |
We fixed it. |
|
Hey, I believe I found the root problem with the build system :) (at least one of them) We compile usb.c to be run on the M0 core of the SOC, but when we specify that macro, we get the warning of redefining the IRQ values, etc... The reason is that in |
@martinling if you are OK with that modification, what do you prefer: a new commit in that PR, or another PR just for the include change ? |
I don't see how that can work, because I've checked, and the If the toolchain has been told that it's building for the M0, then it should fail to assemble that code. So how does it build? If the build system is actually building that code to target the M4, and then the firmware is running it on the M0, then surely it should immediately hard fault when it hits those instructions. So how does it run? It seems to me like your setup can't be doing what you think it's doing. |
I'm still investigating the ldrex / strex situation. We actually get warnings that __ldrex and __strex are being "implicitly declared" in usb_queue.c, we are using it for usb serial only at this point, it might not need those function in that scenario or at least survives without, again, still investigating :)
It IS doing something at least :P But agreed, we need to continue the investigation! Still, IMHO using the dispatch header seems to be the correct way of including nvic.h and it won't change anything in your build. |
That still doesn't explain how this manages to build. Implicit declarations in C are what happens when the compiler sees a function name that hasn't been declared. You get the warnings because your code has not included the declarations of those two functions, which come from uint32_t __ldrex(volatile uint32_t *addr);
uint32_t __strex(uint32_t val, volatile uint32_t *addr);Those declarations are being skipped because The only reason that it doesn't treat those missing declarations as an error is that back in the 1970s, Dennis Ritchie thought that it would be a useful feature for C to just assume that the missing the prototypes are: int __ldrex();
int __strex();So that's wrong, and in any sane world should have already failed. But because someone might want to compile some vintage PDP-11 code from the 1970s, you still have to tell the compiler explicitly if you don't want it to do that ( But this should still fail at the linker step. When linking the object files for your M0 firmware, the linker will try to find the Or at least they shouldn't be there, because it's only possible to build them for the M4. So if they are, then that means your build system must be taking code that was compiled for the M4, and linking it into firmware that's going to run on the M0. Which is bound to cause all sorts of problems - especially if you don't even know that it's getting that wrong.
The dispatch headers are the correct way of including the appropriate nvic.h for code that is written for multiple libopencm3 platforms. Including Some of those assumptions (like the availability of If someone actually does the work of understanding this code and modifying it to work correctly on the M0, then of course we can change the include to use the dispatch header. But just changing whatever makes the warnings go away isn't going to achieve that. |
|
I think it ends up linking successfully because |
|
Ahh, that explains it! And what those helper functions are doing is just disabling interrupts in I don't think it's a good plan to have those helpers being called called My suggestion is that we fix this by creating a new Draft PR for this at #1517. |
|
Meanwhile I've gone ahead and merged the remaining changes from this PR, let's have a new one for any further changes needed. |
|
Thanks you for the merge / the investigations :-) |
That PR is fixing all the following warnings: