CAN_STACK_DISABLE_THREADS is not checked in all occasions(#632)#638
Conversation
… violated, which disables the thread model of the standard C++ library for asynchronous operation of the Isobus stack, fixed with this commit, the bug was found when adding support for the arm-none-eabi platform, since it does not actually have an implementation of the std::thread model.
4b6f950 to
0a143ec
Compare
|
@ad3154, @martonmiklos, @GwnDaan Hi! This PR is ready for review when you have time, thank you. (Failed mac os test : The difference between sessionRemovalTime - sessionUpdateTime and 750 is 7, which exceeds 5, where This is because the test wants to count the system time, which may drift, this error, I believe, does not relate to the changes introduced by this PR, it highlights a not very good approach to time modeling in this environment. A more correct solution would be to have a timer abstraction, which would eliminate the need for additional system calls, among other things. |
Yeah we definitely need to look into that! It's causing quite a lot of headaches with the flaky tests now |
I understand. I appreciate your effort! Maybe we can try to find a toolchain that runs in a GitHub Action to verify that building succeeds at all times. It's easier said than done ofcourse 🥲 |
|
There are (as examples) I am preparing one another PR for supporting these toolchains... (#647) |
The consistency of the compilation flag CAN_STACK_DISABLE_THREADS was violated, which disables the thread model of the standard C++ library for asynchronous operation of the Isobus stack, fixed with this commit, the bug was found when adding support for the arm-none-eabi platform, since it does not actually have an implementation of the std::thread model.
Changes
1. This fix separates the compilation of the VT server part and the rest of the isobus stack functionality, mainly because the server part will not work well in a single-threaded version, but also because the single-threaded version of the build covers a rather specific use case in which we do not actually need the server part in the same the very fact of the build. (From this point of view, the usual way is to divide into different submodules, libraries, and targets, but I'm not doing that here - it would be another task that implicitly covers and solves this problem. I suppose this could become a separate task in tracker if the community decided that it was really necessary. At the moment, I do not think this is necessary, since we are building static libraries, symbols are resolved at the stage of linking to the final binary, we will not see any symbols in this binary that we do not use even if it was built. Another thing is that, of course, the lack of division into sub-modules, of course, makes the compiler work.)
2. The next correction is to replace std::mutex with a Mutex wrapper from isobus utilities in the transport protocol, well, this is necessary here because compilation must be completed successfully, the entire stack is running in single-threaded mode and here we are not interested in synchronization, but in compilation success.
Fixes #632
Tests
unfortunately, it's not easy to test this, in order to identify the problem, you need a toolchains that does not provide an implementation of the standard C++ threading model, one such toolchain may be, for example, the toolchain from ARM (arm-none-eabi) gcc or clang.(relates to "Support for toolchains that doesn't define standard threading C++ model" #639)
Of course, this is the least that can be expected when receiving PR, but so far this is all that I have, I have not deployed a stand with virtual machines for different platforms for full testing due to lack of time.