search

simplefoc / Arduino-FOC

Arduino FOC for BLDC and Stepper motors - Arduino Based Field Oriented Control Algorithm Library
https://docs.simplefoc.com
MIT License
2.02k stars 518 forks source link

Using include guards as flags will come back to bite you #94

Open steph643 opened 3 years ago

steph643 commented 3 years ago

At least one file in SimpleFOC uses the include guard of an external library (here the STM32duino lib) as a flag: https://github.com/simplefoc/Arduino-FOC/blob/master/src/drivers/hardware_specific/stm32_mcu.cpp#L4

This is dangerous. If the lib author (or anyone trying to port the library, as I did) changes the include guard, this will introduce bugs that can be difficult to track. In my case, it silently prevented SimpleFOC to compile in STM32 mode, leading to strange PWM issues.

askuric commented 3 years ago

Hey @steph643, I see your point, but from the point of view of the Arduino ide this way is pretty safe for now. What would be the alternative to this?

steph643 commented 3 years ago

In my opinion, the waterproof approach is to define your own flags (here SIMPLEFOC_AVR and SIMPLEFOC_STM32). Something like this:

// Check AVR boards. List from here:
// https://github.com/backupbrain/ArduinoBoardManager/blob/master/ArduinoBoardManager.h
#if defined(__AVR_ATmega328P__) || defined(__AVR_ATSAMD21G18A__) || defined(__AVR_ATSAM3X8E__) \
        || defined(__AVR_Atmega32U4__) || defined(_AVR_AR9331__) || defined(__AVR_ATmega16U4__) \
        || defined(__AVR_ATmega1280__) || defined(__AVR_ATmega2560__) || defined(_AVR_ATmega328__) \
        || defined(_AVR_ATmega168__) || defined(_AVR_ATmega168V__) || defined(_AVR_ATmega328V__) \
        || defined(_AVR_ATTiny85__) || defined(__AVR_ARCv2EM__) || (__CURIE_FACTORY_DATA_H_)
    #define SIMPLEFOC_AVR
// Check STM32 boards. List from here:
// https://github.com/stm32duino/Arduino_Core_STM32/blob/master/cores/arduino/stm32/stm32_def.h
#elif defined(STM32F0xx) || defined(STM32F1xx) || defined(STM32F2xx) || defined(STM32F3xx) \
        || defined(STM32F4xx) || defined(STM32F7xx) || defined(STM32G0xx) || defined(STM32G4xx) \
        || defined(STM32H7xx) || defined(STM32L0xx) || defined(STM32L1xx) || defined(STM32L4xxf) \
        || defined(STM32L5xx) || defined(STM32MP1xx) || defined(STM32WBxx)
    #define SIMPLEFOC_STM32
#else
    #error "SimpleFOC: your board is not recognized. Please notify us at https://community.simplefoc.com"
    // If you get the above error, the best thing to do is to notify us at https://community.simplefoc.com
    // Alternatively, you can comment out the #error line, uncomment *one* of the following #define and see how it goes:
    //#define SIMPLEFOC_AVR
    //#define SIMPLEFOC_STM32
#endif

This way you are immediately aware of a change in the libraries you depend upon.

If you find the above overkill, I think replacing #if defined(_STM32_DEF_) by #if defined(STM32_CORE_VERSION_MAJOR) would be an improvement. STM32_CORE_VERSION_MAJOR is an actual flag and is less likely to change than _STM32_DEF_.

askuric commented 3 years ago

Ok, it makes sense. I am not sure that this will be included in the next release but we will definitely consider it and probably adapt a similar strategy. Because we will be refactoring the hardware specific code very soon.