Use consistent naming for Serial interfaces #54
Conversation
|
Memory usage change @ 6511709
Click for full report table
Click for full report CSV |
|
@sbhklr if the "Compile Examples" CI workflow needs to be adjusted to not compile the |
Thanks Per! No, I think it should absolutely be compiled, but I forgot to adjust the example. Will do now :-) |
|
Memory usage change @ 8d731ef
Click for full report table
Click for full report CSV |
|
@per1234 Hmm, I was thinking of remapping the Serial for the M4 in the examples using or It pollutes the examples a bit though. Not sure if it's the best approach. How would you go about it? |
|
That is a good consideration! We are using this sketch for two purposes:
Only considering the first, I would say that it's best to:
and leave it up to the user to adjust the example to Serial1 when they want to use it on the Portenta (M4 core) board. However, this will require us to configure the CI workflow to not compile the sketch, meaning we lose that basic check on compatibility of the Scheduler library with the Portenta (M4 Core) board. A workaround for that could be to add a sketch only intended to be used for testing to a dedicated location in the Arduino mbed OS-enabled platforms platform, something like I think something like this was done in the ArduinoIoTCloud library: As for the examples in the |
|
Due to the long standing precedent set by other Arduino boards, I think it would be confusing for I actually prefer for users to be required to adjust the sketch to use Serial1 instead of Serial because this will make it clear to them that they shouldn't expect the code to use the CDC serial port, but rather pins 13/14 just as it would on the Portenta (M7 core) or MKR boards, and similar to any other Arduino board. Consider Sebastian's suggestion from eariler:
It would be confusing for the meaning of |
|
@per1234 and @sbhklr In the future we can add support for Serial to the M4 by piping the messages to the M7. That makes a lot of sense. I can do my way with a The piping will probably use RPC which for me does not work unless I change my library cflags.txt and cxxflags.txt files with |
8d731ef to
6511709
Compare
|
@per1234 We're gonna merge this PR and in a second step figure out a mechanism for mapping Serial on the M4. How do you suggest to proceed with the examples for CI? I like the |
I think that's the best approach.
I would make a sketch that uses the full API of the Scheduler library, and nothing else. Something like this: #include <Scheduler.h>
void setup() {
Scheduler.startLoop(startLoopTask1);
Scheduler.startLoop(startLoopTask2, 123);
Scheduler.start(startTask1);
Scheduler.start(startTask2, 123);
Scheduler.start(startParametricTask1, (void*)"asdf");
Scheduler.start(startParametricTask2, (void*)"zxcv", 123);
Scheduler.yield();
}
void loop() {}
void startLoopTask1() {}
void startLoopTask2() {}
void startTask1() {}
void startTask2() {}
void startParametricTask1(void *bar) {}
void startParametricTask2(void *bar) {}Put it in the dedicated folder for test sketches and then configure the "Compile Examples" workflow to compile it for all three boards: UNIVERSAL_SKETCH_PATHS: test |
There was a problem hiding this comment.
I would like to see the following added to this PR:
- Add your preprocessor code to the
Portenta_Systemlibrary's examples to make them still compile for the Portenta H7 (M4 core) board. - Update "Compile Examples" workflow so the Scheduler library examples are not compiled for the Portenta H7 (M4 core) board. This only requires moving the "
libraries/Scheduler" path from here to there and there
After a discussion with @hpssjellis I concluded that it's confusing the map the serial interfaces to different UART port depending on the core. It's probably more understandable if the M4 doesn't support `Serial` for now, or to map it also to _UART1_. In the future we can add support for `Serial` to the M4 by piping the messages to the M7.
6511709 to
86962f7
Compare
|
@per1234 Github doesn't allow me to re-request a review from you, so I'm just gonna tag you here :-) Can you have a look at the changes? I'd love to get this into the imminent release of 1.2.3 |
|
Memory usage change @ 86962f7
Click for full report table
Click for full report CSV |
There was a problem hiding this comment.
I like this change. Thanks @sbhklr for making it and also to @hpssjellis for the suggestion!
Supersedes #54 When we'll have full support for SerialRPC we can simply replace that line (still need to understand how to notify the error in case CM7 is missing the "companion" code).
Supersedes arduino#54 When we'll have full support for SerialRPC we can simply replace that line (still need to understand how to notify the error in case CM7 is missing the "companion" code).
After a discussion with @hpssjellis I concluded that it's confusing to map the serial interfaces to different UART port depending on the core. It's probably more understandable if the M4 doesn't support
Serialfor now, or to map it also to_UART1_. In the future we can add support forSerialto the M4 by piping the messages to the M7.