andysworkshop / stm32plus

The C++ library for the STM32 F0, F100, F103, F107 and F4 microcontrollers
http://www.andybrown.me.uk
Other
745 stars 224 forks source link

Fix misbehaving weak MillisecondTimer::delay function #171

Closed iromero91 closed 8 years ago

iromero91 commented 8 years ago

For some reason when I try this library with my compiler [arm-none-eabi-g++ (15:4.9.3+svn231177-1) 4.9.3 20150529 (prerelease)] and the cmake toolchain it seems to cause the delay() function to return immediately (perhaps because it links to the wrong thing). The inline keyword is not very useful anyway for a function defined in the cpp file.

The problem was introduced in pull request #169.

mikepurvis commented 8 years ago

+0

I see no harm in this if it fixes an issue for you. But it would be great to understand why the linker is doing the wrong thing, in case there's a deeper root cause.

iromero91 commented 8 years ago

I'd like to know too, only thing i can think of is that I stumbled upon a GCC bug, I don't recall having a function with the inline attribute defined in the .cpp being undefined behavior, then again, the weak symbol machinery is not in the C++ standard.

Oh i found some evidence that it might be indeed a GCC quirk https://gcc.gnu.org/ml/gcc-help/2014-02/msg00162.html

To be honest I think the best way going forward for this kind of stuff is to do overrides at the source level and switch from globally built library to something that can be inserted in your project as a git submodule.

mikepurvis commented 8 years ago

Compile time ifdefs is certainly a standard practice in the embedded C world— that's for sure how libraries like FreeRTOS and LwIP are configured. That said, I really appreciate the amount of configuration that is possible in stm32plus using templates to still deliver relatively compact binaries.

This approach makes library a good fit for being able to use pre-compiled binaries of stm32plus, which works well in my company's projects, and that ability breaks down once there are project-specific switches that need to be set at compile time.

Indeed, I even wonder if it might be possible to eliminate specifying the HSE/HSI at compile time by having a weakref function return that value, which can be overridden by the project, though that discussion is getting out of the scope of this ticket. :)

andysworkshop commented 8 years ago

I'd like to see the linker output that shows what's actually been linked to when the problem occurs but nevertheless I see no problem with the pull request as-is. Merging.