Skip to content

Bugs: Typical sketches that do not compile with arduino-cli (MT) #43

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

Closed
aentinger opened this issue Oct 22, 2021 · 6 comments · Fixed by #47
Closed

Bugs: Typical sketches that do not compile with arduino-cli (MT) #43

aentinger opened this issue Oct 22, 2021 · 6 comments · Fixed by #47
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@aentinger
Copy link
Contributor

This issue contains a list of sketches which contain typical programming paradigms which break when compiling with the current version of the multi-threading supporting arduino-cli, due to the relatively simple wrapping of the whole file within a class.

1) .inot contains both function definition and declaration

Breaks_1.zip

2) Failure to register callback since all functions within the .inot` file are effectively member functions (and not free functions)

Breaks_2.zip

3) Initialisation of static variable fails (due to being a class member instead of a default global variable)

Breaks_3.zip

4) Instantiating any kind of object with constructor within the class fails (obviously)

Breaks_4.zip

One possible fix for all those issues would be to directly instantiate the threading class within the start of the inot file, then only prefix setup/loop with class name, i.e.

+class Thread_1 : public Arduino_Threads
+{
+public:
+  Thread_1 { _tabname = "Thread_1" }
+};

-void setup() {
+void Thread_1::setup() {

-void loop() {
+void Thread_1::loop() {


+Thread_1 thread_1;

@facchinm What do you think? Can you rework the arduino-cli branch to support this?
CC @pnndra

@aentinger aentinger added the type: imperfection Perceived defect in any part of project label Oct 22, 2021
@facchinm
Copy link
Collaborator

facchinm commented Oct 22, 2021

So, that's in fact a lot 🙃

  1. I'm not sure we can do anything without calling ctags beforehand, recognizing the function definition as such and removing its lines (not easy at all)
  2. Automatic recognition looks HARD, but I'd add a commodity macro like CALLBACK(fn) thet wraps mbed::callback(this, &func). Anyway, the user should explicitly call it.
  3. Is there any valid use of static keyword in Thread wrappers "as we want"? If not, I can patch the builder to remove the keyword entirely
  4. no clue on how to solve it

The proposal about the threading class looks interesting but I'm thinking about how it copes with variables (which is the thing we want to protect). Adding Thread_1::variable requires it to be declared in the class with obvious disadvantages

@aentinger
Copy link
Contributor Author

Hi @facchinm 👋 ☕ 🐐

It sure looks like a lot but in fact every single item results from the fact that the whole *.inot file is turned into a C++ class declaration (due to arduino-cli inserting THD_ENTER at the beginning of each *.inot file and THD_DONE at the end of each *.inot file).

But in my understanding this is not a problem. Instead I think this is fantastic because we only need to fix one thing instead of coming up with work-arounds and solutions for each of the 4 cases I listed above (and there may be more which I've simply not uncovered yet).

Now, I don't understand how arduino-cli works but how difficult would it be to just insert one macro at the start of the file, i.e.

/* Arduino_Threads.h */
#define THD_ENTER(tabname) class ARDUINO_THREADS_CONCAT(tabname, Class) : public Arduino_Threads { \
public: \
  ARDUINO_THREADS_CONCAT(tabname, Class)() { _tabname = ARDUINO_THREADS_TO_STRING(tabname); } \
protected: \
  virtual void setup() override; \
  virtual void loop () override; \
};  \
ARDUINO_THREADS_CONCAT(tabname,Class) tabname;

/* MyThread.inot */
THD_ENTER("MyThread");

and to just prefix setup()/loop() with the thread-name/class-name?

-void setup() {
+void MyThread::setup() {

-void loop() {
+void MyThread::loop() {

Doing so would eliminate all issues introduced by putting every *.inot file within a class.

@facchinm
Copy link
Collaborator

My main concern is due to variable, which can't be prefixed (it would change their scope) and we don't want them to be available to other parts of the "project" (and this is enforced by the private section)

@aentinger
Copy link
Contributor Author

aentinger commented Oct 27, 2021

This could still be solved by encapsulating the whole file in namespace, i.e.

+namespace MyThreadNamespace {
/* ... */
+} /* MyThreadNamespace */

@facchinm
Copy link
Collaborator

Would you mind creating a PoC of the resulting "preprocessed" code so I can reproduce it in the cli? I'm still not 100% it's working as expected 🙂

@aentinger
Copy link
Contributor Author

Dear Community!

The way multi-threading for Arduino currently works is by wrapping the complete content of an inot-file within the class body of a special C++ class. This comes with certain disadvantages as begin discussed in this thread. There is a solution for most of these problems by instead of encapsulating the whole file within a class body you just encapsulate the whole inot-file within a namespace (see #47). Alternately you can either write your code in a different way (see https://github.com/arduino-libraries/Arduino_Threads/pull/52/files).

What's your opinion on this subject? Would you prefer inot-files to be realised via class-body or via namespace?

@per1234 per1234 added the conclusion: resolved Issue was resolved label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
3 participants