Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32F1: Dummy SoftwareSerial (as TMCStepper fallback) #14861

Merged
merged 3 commits into from
Aug 8, 2019

Conversation

tpruvot
Copy link
Contributor

@tpruvot tpruvot commented Aug 6, 2019

Latest TMCStepper lib includes a lib not available on the F1 for Arduino... so lets simulate it for now...

@tpruvot tpruvot changed the title STM32F1: fake SoftwareSerial class to compile with last TMCStepper ve… STM32F1: fake SoftwareSerial class to compile TMCStepper Aug 6, 2019
@tpruvot tpruvot changed the title STM32F1: fake SoftwareSerial class to compile TMCStepper STM32F1: fake SoftwareSerial to build TMCStepper lib Aug 6, 2019
@tpruvot tpruvot mentioned this pull request Aug 6, 2019
Marlin/Configuration.h Outdated Show resolved Hide resolved
@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 6, 2019

sorry, rebased on previous commit to show it can build with it (travis)

@thinkyhead
Copy link
Member

thinkyhead commented Aug 6, 2019

I would rather avoid hacks like this, if a simpler method will work.

I'm working diligently within #14832 to determine whether __has_include applied to TMCStepper will fix the problem. So far it looks promising, but I'm testing all the STM32 platform builds to see what the results are. It's a bit slow-going, because the MKS_ROBIN pins file sets SS_PIN to -1 and the STM32F1 HAL specifically rejects that. And I expect other platforms to have similar snags to solve.

@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 6, 2019

Yes , tpruvot . At the moment if i want to make my STM32F1 board works , i have to build a lib for the software serial. https://github.com/FYSETC/SoftwareSerialM

didnt test yet, will check if that build with it

@thinkyhead
Copy link
Member

So far it solves the issue for STM32F103R and MKS_ROBIN… Running mftest on all the others and will report soon…

@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 6, 2019

ok, doing a pause anyway.. the repo need some cleanup to do anything more...

@tpruvot tpruvot changed the title STM32F1: fake SoftwareSerial to build TMCStepper lib STM32F1: empty SoftwareSerial to build TMCStepper lib Aug 7, 2019
Its required to compile with last TMCStepper lib version.

TODO: implement https://github.com/FYSETC/SoftwareSerialM (only if wanted)
@thinkyhead
Copy link
Member

Looks like @teemuatlut updated TMCStepper. So now, even without any changes, most of the tests are passing Travis CI. However, the two STM32 tests are still failing. This PR does prevent the compiler error, which is why it's the only PR now passing tests. But, maybe we can just get all the STM32 builds to start using C++17…?

@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 7, 2019

rmm, so this has_include will randomly use a lib which doesnt exist, even if not using TMC drivers at all ?

@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 7, 2019

i was more planning to include the needed code in the F1 HAL and enable it (require an extra timer) only with TMC220x_UART enabled

@teemuatlut
Copy link
Member

Either

  • we remove STM from the manually populated list of SW Serial capable platforms
  • we list a compatible SW Serial implementation as a dependency for the lib
  • we update STM to use C++17

@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 7, 2019

What [worries] me the most is not the C++17 standard on a compiler dated from 2017, but the fact the lib may use dangerous ressources outside the tracked source code (like hardware timers or worst).

@thinkyhead
Copy link
Member

rmm, so this has_include will randomly use a lib which doesnt exist, even if not using TMC drivers at all ?

PlatformIO limitation to the way it includes libraries. If a library may be used in your code, but you're not sure, you cannot (easily) get the library to not compile. So, the TMCStepper library is compiled for every environment that includes it. So it is up to the library itself to manage the problems that could arise from trying to include stuff that doesn't exist for the current platform.

@thinkyhead
Copy link
Member

i was more planning to include the needed code in the F1 HAL and enable it (require an extra timer) only with TMC220x_UART enabled

So, in other words, you want to do a full implementation of SoftwareSerial within Marlin for the platforms that don't supply it? That would be great! Meanwhile, we'll still want to get TMCStepper.cpp set up to be able to decide whether or not it should try to include SoftwareSerial at all.

There is something useful we can definitely do here with this dummy library to help out TMCStepper! We just need to add a #define inside of SoftwareSerial.h that tells TMCStepper it's a DUMMY_SW_SERIAL and not a real one, and then it can know that these SoftwareSerial classes are just placeholders.

@thinkyhead
Copy link
Member

Either

  • we remove STM from the manually populated list of SW Serial capable platforms
  • we list a compatible SW Serial implementation as a dependency for the lib
  • we update STM to use C++17
  • We add a flag to SoftwareSerial.h here so you can see it's a dummy library.

@thinkyhead
Copy link
Member

What [worries] me the most is not the C++17 standard on a compiler dated from 2017, but the fact the lib may use dangerous resources outside the tracked source code (like hardware timers or [worse]).

Is the first issue (old compiler) going to produce a crappier binary?

Is this a specific SoftwareSerial concern or a wider concern about other libraries? And are you also including any of the base platform libraries in that group?

@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 8, 2019

else gnu++17 seems to work the same... but the TMCStepper hasinclude has been pushed on the wrong branch...

@thinkyhead
Copy link
Member

I see it on TMCStepper/master, and a version bump to 0.4.6.

@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 8, 2019

yep, in fact the hasinclude commit is after the 0.4.6 tag

@thinkyhead
Copy link
Member

So it is! I think it's just waiting for some confirmation, and then it'll get a tag.

@thinkyhead
Copy link
Member

Also added teemuatlut/TMCStepper#57

@thinkyhead thinkyhead merged commit 535018e into MarlinFirmware:bugfix-2.0.x Aug 8, 2019
@thinkyhead thinkyhead changed the title STM32F1: empty SoftwareSerial to build TMCStepper lib STM32F1: Dummy SoftwareSerial (as TMCStepper fallback) Aug 8, 2019
@teemuatlut
Copy link
Member

tells TMCStepper it's a DUMMY_SW_SERIAL

I don't think I'd need to act on this in any way. The TMCStepper can make the calls to your dummy SW Serial as per usual but they just don't do anything.
All this then eventually just boils down getting stuff to compile.

  #ifdef SW_SERIAL_PLACEHOLDER
    #undef SW_CAPABLE_PLATFORM
  #endif

With this all your SoftwareSerial.h header needs to include is the SW_SERIAL_PLACEHOLDER define, as any calls to a SoftwareSerial class then don't get compiled and thus no need to even have the dummy class.

@tpruvot
Copy link
Contributor Author

tpruvot commented Aug 8, 2019

@teemuatlut yes, without the #undef, we could track/log what is sent to the TMC to debug... later. Anyway, travis/users problem should be resolved for the F1s for now... To be continued for real UART support...

@tpruvot tpruvot deleted the stm32f1_tmc_serial branch August 8, 2019 07:49
@teemuatlut
Copy link
Member

@thinkyhead I don't currently think I need to merge in your PR. This change here makes it possible for me include a "working" (=compiling) SW Serial for STM, but then your PR instructs me to not use any of it. Ok if we go with just this dummy version for now with no further changes to TMCStepper?


Ultimately my goal is that TMCStepper would be able to use any installed SoftwareSerial implementation it can find (as long as API compatible), but if the header is not found, then disable the functionality. This should be doable with the __has_include and when coupled with = delete we can have a easily understandable error message when trying to call functionality that's not supported.
I think it might be possible (with PIO) to specify C++17 just for the TMCStepper library and with this cause more mayhem and mischief!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants