Skip to content

Arduino_FreeRTOS: automatically start loop #369

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

Merged
merged 1 commit into from
Nov 28, 2024

Conversation

facchinm
Copy link
Member

If anyone can find a better name for eventually_start_scheduler_and_sketch_task I'd be very happy 🙂

@facchinm facchinm requested a review from aentinger August 28, 2024 08:56
@per1234 per1234 added the topic: code Related to content of the project itself label Aug 28, 2024
@pennam pennam force-pushed the freertos_wrap_loop branch 2 times, most recently from 9f7de8e to fee8ca9 Compare November 27, 2024 10:28
@pennam
Copy link
Contributor

pennam commented Nov 27, 2024

Thanks @alrvid for the name suggestion 🙂

@pennam pennam marked this pull request as ready for review November 27, 2024 10:28
@pennam pennam merged commit 428df2a into arduino:main Nov 28, 2024
6 of 7 checks passed
@gpb01
Copy link

gpb01 commented Dec 5, 2024

I really don't understand this change ... on all platforms and in all environments, the task of starting the FreeRTOS™ scheduler is up to the user, by calling the vTaskStartScheduler() function at the end of all the preparation, so ... why would you want to 'ape' the (practically useless) AVR version of the Arduino_FreeRTOS library and not, in a much more professional (and educational) way, leave the task of starting the scheduler up to the user, as is done everywhere?

To turn everything into a 'toy' again?

Guglielmo

@hexnet1234
Copy link

In my opinion, this change should be reverted, this breaks so much...

Issue 1:
The Loop Thread uses 4k of RAM. The Arduino R4 only has 32k of RAM, and we cannot assume that the User Application still has 4k of RAM that it can just "throw away". The loop function might not even be used, so those 4k of RAM are just wasted.
I could not even get all my Tasks to initialize without turning this down.

Issue 2:
The Priority is set to the highest possible priority!!!
For example, in my code I am creating multiple Tasks, all at priority 1. The Loop Task will starve all other tasks with lower priority of CPU time, so they will never run. (USE_PREEMPTION is enabled)

Issue 3:
The scheduler absolutely must not be started before initialization is complete.
For example in my code: In setup() I create some tasks using xTaskCreate and then some queues using xQueueCreate, after creating the tasks. However, by starting the scheduler before the setup() function is called, and with USE_TIME_SLICING enabled, the tasks are started by the scheduler, before the queues are properly initialized, causing the tasks and the Arduino to crash when the task tries to access the queue.
Again: The scheduler absolutely must not be started before initialization is complete. Initialization of the resources needed by the Tasks is up to the User, and therefore starting the scheduler should also be done by the user, after all initialization is complete.

In my brutally honest optinion, this change should be reverted, or at the very least add an option for this in FreeRTOSConfig.h!

@gpb01
Copy link

gpb01 commented Dec 8, 2024

In my brutally honest optinion, this change should be reverted, or at the very least add an option for this in FreeRTOSConfig.h!

I agree with you 100% and, If you can read Italian or use Google Translate, you can see HERE, which is exactly what I did and suggested to the readers of the Italian forum.

Guglielmo

@facchinm
Copy link
Member Author

facchinm commented Dec 9, 2024

Hi @gpb01 and @hexnet1234, I understand your concerns so I'll try to explain the rationale.

Some libraries (like the networking restructuring for C33 we are working on) will need to use threads and thus the provided FreeRTOS wrapper; this should be an implementation detail, so the user will only need to see what happens "in the foreground" via the normal library APIs

If we don't add an hook to start the scheduler and to automatically launch loop(), this might end up in a situation where:

  • either the user forgets to call vTaskStartScheduler and nothing works
  • the user calls vTaskStartScheduler without realizing that everything after that line will never be run

IMO, neither of these options is sensible; on the other hand, RAM in UNO R4 is limited indeed, so wasting 4K is not acceptable. Second, the concern about spawning other threads after vTaskStartScheduler is very correct and should be avoided.

Proposed solution:

  • Leave the code for C33 only (since it's not necessary for anything UNO R4 related) and move start_freertos_on_header_inclusion() after setup(), so any task can be created correctly while keeping the same ideas.

What do you think?

@hexnet1234
Copy link

Hi @facchinm,

Unfortunately, I am not familiar with the C33, so I cannot comment on how to implement it for this system.

However, for the Uno R4s, this change seems reasonable to me. I reckon a simple check like #if defined(ARDUINO_PORTENTA_C33) around the lines you added for this commit would be absolutely enough.

My concern is that, with the Portenta C33, the same issues with wasting RAM and spawning Tasks after starting the Scheduler may still occur. And what about the Task being at priority level 4?
Unfortunately, as I've said above, I am not familiar with the C33, so I cannot suggest any improvements, and maybe my concerns are not relevant for the C33.

Regardless, I am curious to see what you come up with.

Best regards
hexnet1234

@gpb01
Copy link

gpb01 commented Dec 9, 2024

@facchinm:

I know you speak Italian, so as it's not easy to translate into English, I'll write it in Italian for you:

"probabilmente, si ritiene che gli "utenti" siano in realtà degli "utonti" ...

... this is what comes to mind when reading the reasons for the change.

As I have written several times, the use of FreeRTOS requires prior study and ... one of the fundamental things you learn by studying any FreeRTOS text is that the "vTaskStartSchedule"r function is the last one that must be called, when everything is ready and that, from that moment, control passes to the RTOS, so ... anyone who uses FreeRTOS, especially in the professional field, knows this and certainly does NOT need a dis-educational and harmful modification. For me, that change should be totally eliminated, it only creates problems and, I repeat, it is also uneducational.

I do not use 'Portenta' boards, but I think they are for professional users who spend time studying a product before using it, so ... they know what they have to do.

Sincerely,
Guglielmo

@gpb01
Copy link

gpb01 commented Dec 9, 2024

Some libraries (like the networking restructuring for C33 we are working on) will need to use threads and thus the provided FreeRTOS wrapper; this should be an implementation detail, so the user will only need to see what happens "in the foreground" via the normal library APIs

So ... ONLY for "Some libraries" you are going to use FreeRTOS™ ... therefore, if a user uses certain libraries he will automatically find himself in a FreeRTOS™ environment (preconfigured by you), otherwise no ... it does not seem a good idea at all ... rather take a cue from what Espressif does with ESP32, in which the user is ALWAYS in a FreeRTOS™ environment (and the thing is well documented).

Cordially,
Guglielmo

@facchinm
Copy link
Member Author

facchinm commented Dec 9, 2024

@gpb01 it's not about "utenti" or "utonti" but more about hiding implementation details; at the end of the day, the user will want to call Client.write(), without even knowing that the queue is handled by lwip in its own thread.

ESP32 does this in fact and I think it's a good approach; C33 could, as you suggest, ALWAYS run in a freertos aware environment, but in that moment it would be beneficial to move it to its own core (that was not in the original plan).

If you agree, I would restrict the change to C33 only at the moment via one simple #ifdef ARDUINO_PORTENTA_C33 guard, and think about keeping the behaviour on by default via some heavier modifications (always, for C33 only)

@gpb01
Copy link

gpb01 commented Dec 9, 2024

If you agree, I would restrict the change to C33 only at the moment via one simple #ifdef ARDUINO_PORTENTA_C33 guard, and think about keeping the behaviour on by default via some heavier modifications (always, for C33 only)

@facchinm: As mentioned, I do not use "Portenta", so ... initially it could be a possible solution waiting for an evolution similar to the ESP32, but ... pay attention to the points indicated by hexnet1234 (stack, priority, ecc.).

For UNO R4, please, completely eliminate the modification and restore the previous situation.

Sincerely,
Guglielmo

@hexnet1234
Copy link

I personally, as an R4 user, would be completely fine with an #ifdef ARDUINO_PORTENTA_C33 guard,
please do implement this.

For the C33, I personally don't really care since I don't use it.

However, implementing such a change will certainly not be very easy.
If the priority, of for example a Network Task, is too high, it will block User Tasks. If it is too low, it will never run,
and Networking will not work. I am no expert in FreeRTOS, so I am not sure if it is possible to (easily) work around this problem.

As I said, I am very curious to see what you come up with.

Best of luck with implementing the Library changes for the C33!

Best regards
hexnet1234

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants