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

Added support for ESP32 with Husarnet TCPv6 transport #708

Open
wants to merge 6 commits into
base: galactic
Choose a base branch
from

Conversation

DominikN
Copy link
Contributor

No description provided.

@pablogs9
Copy link
Member

pablogs9 commented Jan 21, 2022

Hello @DominikN, thanks a lot for the contribution.

Can you detail the reasons why the old ESP toolchain is needed?
Related: micro-ROS/docker#76

@Acuadros95 in general this LGTM, what about all the style change at src/micro_ros_arduino.h?

@pablogs9 pablogs9 requested review from pablogs9 and Acuadros95 and removed request for pablogs9 January 21, 2022 07:08
@pablogs9
Copy link
Member

@mergify backport main foxy

@mergify
Copy link
Contributor

mergify bot commented Jan 21, 2022

backport main foxy

🟠 Waiting for conditions to match

  • merged [:pushpin: backport requirement]


/* Try to connect to a server on port 8888 on your laptop */
if (!client.connect(locator->hostname, locator->port)) {
Serial1.printf("failed\r\n");
Copy link
Member

Choose a reason for hiding this comment

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

Also here

find firmware/build/include/ -name "*.c" -delete
cp -R firmware/build/include/* /project/src/

mkdir -p /project/src/esp32_5_2_0
Copy link
Contributor

@Acuadros95 Acuadros95 Jan 21, 2022

Choose a reason for hiding this comment

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

Will Arduino IDE look in this path for the library?

Copy link
Contributor Author

@DominikN DominikN Jan 21, 2022

Choose a reason for hiding this comment

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

Only with additional flags:

-L <path_to_microros>/src/esp32_5_2_0/
-l microros

There is still esp32 folder that will be handled by default by Arduino IDE

@Acuadros95
Copy link
Contributor

In general I agree with @pablogs9:

  • It would be nice if the same toolchain could be used for all ESP projects
  • We should get rid of the style change of src/micro_ros_arduino.h, so the PR looks cleaner
  • I would leave stdout traces out, or maybe add a conditional debug interface in the future

@DominikN
Copy link
Contributor Author

Hi guys, in the micro-ROS/docker#76 - I added a toolchain for building microros static library based on the same toolchain version as used for a current version of Husarnet for ESP32.

We are in the process of making Husarnet working as a standard Arduino library working with the latest releases of https://github.com/espressif/arduino-esp32 . Right now Husarnet uses this fork: https://github.com/husarnet/arduino-esp32. Main issue behind using a custom fork was a poor support of IPv6 in the past, but it has changed (also due our PRs) and in the following months we will get rid of our fork.

By this time I will also make a PR to get rid of esp32_5_2_0 folder (and building with older toolchain) in this repo.

I would like to present Husarnet, micro-ROS and ESP32 during my presentation at ROS 2 Embedded Working Group working with micro-ROS based on the https://github.com/micro-ROS/micro_ros_arduino and not on my fork.

@DominikN
Copy link
Contributor Author

DominikN commented Jan 21, 2022

In general I agree with @pablogs9:

* It would be nice if the same toolchain could be used for all ESP projects

* We should get rid of the style change of `src/micro_ros_arduino.h`, so the PR looks cleaner

* I would leave stdout traces out, or maybe add a conditional debug interface in the future
  1. This is a temporary workaround, that will not brake anything but will allow to make working Husarnet TCPv6 transport in platformio projects out of the box. In the previous message I wrote why I did it.
  2. Yea, sorry guys, my auto code formatting settings where different than yours. What code formatter do you use? Everybody have different prefferences when it comes to code editor, but what do you think about providing settings for VSC in the repo (in .vscode folder that is automatically handled by VSC after opening a project)? Something like that I have added to one of my previous projects: https://github.com/DominikN/ros2_docker_examples/tree/main/.vscode
  3. Totally agree.

@pablogs9
Copy link
Member

Hi @DominikN,

we have been discussing this internally and the best option is to wait until we can have this built with the mainline compiler version. As soon as this transport is compatible with the version of the ESP32 compiler we will merge this.

Regarding the EWG, do not worry about using local branches, we can say that all the shown features will be available as soon as the compiler issue is solved.

@DominikN
Copy link
Contributor Author

Sure, that's fine. Thanks for your reply.

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