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

Add support for Arduino Due and Arduino Zero #34

Merged
merged 11 commits into from
Nov 19, 2020

Conversation

lukicdarkoo
Copy link
Contributor

This PR adds support for Arduino Due and Arduino Zero.

@@ -46,3 +47,4 @@ jobs:
arduino-cli compile --fqbn OpenCR:OpenCR:OpenCR /github/home/Arduino/libraries/micro_ros_arduino-0.0.1/examples/micro-ros_publisher -v
arduino-cli compile --fqbn teensy:avr:teensy31 /github/home/Arduino/libraries/micro_ros_arduino-0.0.1/examples/micro-ros_publisher -v
arduino-cli compile --fqbn teensy:avr:teensy41 /github/home/Arduino/libraries/micro_ros_arduino-0.0.1/examples/micro-ros_publisher -v
arduino-cli compile --fqbn arduino:samd:arduino_zero_native /github/home/Arduino/libraries/micro_ros_arduino-0.0.1/examples/micro-ros_publisher -v
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI tests are missing here as the upstream version of Arduino Due core doesn't support static libraries. Here is a relevant PR:
arduino/ArduinoCore-sam#115

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm curious to know why they allow precompiled linking and dont have to support in their own boards

@@ -0,0 +1,50 @@
#include <Arduino.h>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to be able to change the transport in the project instead of the library. MCUs usually have multiple UARTs that users may switch to, or to other protocols, SPI, I2C, and CAN. Also, we can benefit from Arduino's libraries to interact with those protocols. Defining transport in the project would be super handy.

However, in that case, we need to include the same transport file in all examples which is not the best solution. Let me know if you have any suggestions.

&& rm -rf gcc-arm-none-eabi-5_4-2016q3-20160926-linux.tar.bz2 gcc-arm-none-eabi-5_4-2016q3/share/doc \
&& wget https://launchpad.net/gcc-arm-embedded/4.8/4.8-2014-q1-update/+download/gcc-arm-none-eabi-4_8-2014q1-20140314-linux.tar.bz2 \
&& tar -xvf gcc-arm-none-eabi-4_8-2014q1-20140314-linux.tar.bz2 \
&& rm -rf gcc-arm-none-eabi-4_8-2014q1-20140314-linux.tar.bz2 gcc-arm-none-eabi-4_8-2014q1/share/doc
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test, but probably we can recycle the same compiler for multiple platforms. I picked a safe route and took the same compiler that Arduino uses for the corresponding cores.

Comment on lines +42 to +45
"-DRMW_UXRCE_MAX_PUBLISHERS=2",
"-DRMW_UXRCE_MAX_SUBSCRIPTIONS=1",
"-DRMW_UXRCE_MAX_SERVICES=0",
"-DRMW_UXRCE_MAX_CLIENTS=1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without these changes, I wasn't able to fit the library into Arduino Zero. Ideally, we would also be able to change these parameters from an Arduino project (without changing and recompiling the library), but I didn't know how to solve it.

@@ -5,11 +5,8 @@
#include "teensy_transports.c.in"
#elif defined(ARDUINO_ARCH_OPENCR)
#include "opencr_transports.c.in"
#else
#error micro-ROS Library not supported for this platform
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transport is defined in the Arduino project and we don't need these files here. Should we move the OpenCR and Teensy transport files?

@lukicdarkoo
Copy link
Contributor Author

@pablogs9 I was indecisive about a few things, so let me hear your thoughts and we can fix CI.

@pablogs9
Copy link
Member

Hello @lukicdarkoo, your comments are really interesting. I'm answering here:

  • Regarding the CI, I'm going to patch the required files in the same way we are doing with the Teensy Ones here and here. When they merge Include compiler.libraries.ldflags property arduino/ArduinoCore-sam#115, we can fix it. Regarding this, I'm also going to add instructions in the Readme.

  • Regarding the transports, I'm completely ok with providing a user-level transport layer but I don't want to remove the default one. So, in my opinion, we should merge this PR and open a new one restructuring around this idea. The first idea that came to my mind is to use weak functions symbols that can be overridden. So I'm going to keep the transports as they are and we can re-architecture them in another PR.

  • Regarding the compilers, for sure the "safe route" will end in a tons of gigas Docker container, I don't like it. I'm going to test if can use at least the latest release of each major version of GCC. If my tests are successful we can start to remove duplicated toolchains.

  • Regarding your colon-verylowmem.meta, I'm ok with it. Regarding changing this configuration at runtime, I think that it is not possible due to the architecture of the micro-ROS middleware, which is based on static memory pools. You can find information about this in the closed issues of this repo and here. If you have any idea for tunning this without the use of dynamic memory, or at least, having a static memory part for applications that need hard real-time, we are open to discussion.

@pablogs9 pablogs9 merged commit bdcad0b into micro-ROS:foxy Nov 19, 2020
@pablogs9
Copy link
Member

Ok, I'm going to add a commit to this with:

  • Fixed CI and included new platforms
  • Added your Serial transport to the library as weak functions that can be overridden by:
    • User-provided transport at the application level
    • Our oldies transports for OpenCR and Teensy transports: This should be deprecated and we should think about a generic transport architecture that can be overridden by the user and selectable in the library
  • Added example with custom transport
  • Added patching instructions for SAM
  • Updated README.md with contributed boards. @lukicdarkoo let me know if you are ok with this.

@pablogs9
Copy link
Member

Hey @lukicdarkoo, I have merged and integrated all your contributions. Check release v0.0.4.

Thanks a lot for this. If you keep working on this and improving the micro-ROS for Arduino we are super happy to receive your PR.

@lukicdarkoo
Copy link
Contributor Author

Regarding the CI, I'm going to patch the required files in the same way we are doing with the Teensy Ones here and here. When they merge arduino/ArduinoCore-sam#115, we can fix it. Regarding this, I'm also going to add instructions in the Readme.

Thanks for this!

Regarding the transports, I'm completely ok with providing a user-level transport layer but I don't want to remove the default one. So, in my opinion, we should merge this PR and open a new one restructuring around this idea. The first idea that came to my mind is to use weak functions symbols that can be overridden. So I'm going to keep the transports as they are and we can re-architecture them in another PR.

This sounds like a very good solution.

Regarding your colon-verylowmem.meta, I'm ok with it. Regarding changing this configuration at runtime, I think that it is not possible due to the architecture of the micro-ROS middleware, which is based on static memory pools. You can find information about this in the closed issues of this repo and here. If you have any idea for tunning this without the use of dynamic memory, or at least, having a static memory part for applications that need hard real-time, we are open to discussion.

I was thinking about this, but, unfortunately, I cannot think of any good alternative. However, I believe there is a huge need to solve this, so we should keep thinking about possible solutions.

@flynneva
Copy link

Regarding your colon-verylowmem.meta, I'm ok with it. Regarding changing this configuration at runtime, I think that it is not possible due to the architecture of the micro-ROS middleware, which is based on static memory pools. You can find information about this in the closed issues of this repo and here. If you have any idea for tunning this without the use of dynamic memory, or at least, having a static memory part for applications that need hard real-time, we are open to discussion.

I was thinking about this, but, unfortunately, I cannot think of any good alternative. However, I believe there is a huge need to solve this, so we should keep thinking about possible solutions.

i'm not the person who would be able to answer this but I feel like there is a possibility that the RMW that my friend @schilasky is working on over at this repo rmw_ecal might be able to help with? it uses ecal as its middleware...which might help with memory management. @schilasky is much more knowledgeable though.

there is also rmw_iceoryx which might also be a viable solution? again....i dont really know what im talking about here so i might be completely off that either of these can actually help.

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

Successfully merging this pull request may close these issues.

3 participants