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

FreeRTOS-Plus-TCP port #239

Merged
merged 41 commits into from
Oct 30, 2023
Merged

FreeRTOS-Plus-TCP port #239

merged 41 commits into from
Oct 30, 2023

Conversation

bjsowa
Copy link
Contributor

@bjsowa bjsowa commented Aug 11, 2023

Features to implement:

  • UDP Unicast transport
  • Build tests
  • UDP Multicast transport
  • TCP transport
  • Multi threading
  • (optional) IPv6 support

@bjsowa bjsowa mentioned this pull request Aug 11, 2023
@cguimaraes
Copy link
Member

@bjsowa check failing test regarding the Eclipse ECA. Otherwise, we will not be able to merge this PR.

Do you have a timeline for the remaining features?

@bjsowa
Copy link
Contributor Author

bjsowa commented Aug 12, 2023

@bjsowa check failing test regarding the Eclipse ECA. Otherwise, we will not be able to merge this PR.

Fixed

Do you have a timeline for the remaining features?

I don't.

Copy link
Member

@cguimaraes cguimaraes left a comment

Choose a reason for hiding this comment

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

A first quick review.
Tests must be also provided...at least build tests.

src/system/freertos_plus_tcp/system.c Show resolved Hide resolved
ret = _Z_ERR_GENERIC;
}

ep->_iptcp->ai_addr->sin_family = FREERTOS_AF_INET4;
Copy link
Member

Choose a reason for hiding this comment

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

isn't this value set by getaddrinfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not, getaddrinfo only sets ai_family but does not write it to ai_addr->sin_family. I changed it so it at least uses the value returned by getaddrinfo.

src/system/freertos_plus_tcp/system.c Show resolved Hide resolved

unsigned long z_clock_elapsed_ms(z_clock_t *instant) { return z_time_elapsed_ms(instant); }

unsigned long z_clock_elapsed_s(z_clock_t *instant) { return z_time_elapsed_ms(instant) * 1000; }
Copy link
Member

Choose a reason for hiding this comment

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

This will return seconds but useconds instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it. It is worth noting that the emscripten port, which I used for reference, has the same issue.

@bjsowa
Copy link
Contributor Author

bjsowa commented Aug 28, 2023

@cguimaraes I added TCP support, some examples and CI workflow. Could you review the changes?

The examples use POSIX port of FreeRTOS and network interface implementation that uses SLiRP library to emulate a userspace NAT network. I was able to use the examples on Linux in client mode with a Zenoh Router running on the host. Peer mode won't work as this port does not support incoming connections.

Signed-off-by: Błażej Sowa <[email protected]>
Signed-off-by: Błażej Sowa <[email protected]>
Signed-off-by: Błażej Sowa <[email protected]>
Signed-off-by: Błażej Sowa <[email protected]>
Signed-off-by: Błażej Sowa <[email protected]>
Signed-off-by: Błażej Sowa <[email protected]>
@bjsowa
Copy link
Contributor Author

bjsowa commented Aug 29, 2023

@cguimaraes I added Multi threading support and extended Zenoh-pico API to allow passing port-specific task attributes when creating read and lease tasks (example usage in z_pub.c). Let me know what you think about it.

Multicast is not fully supported in FreeRTOS-Plus-TCP (see FreeRTOS/FreeRTOS-Plus-TCP#962) so I'll have to pass on UDP Multicast for now. I'll also pass on IPv6 support as I don't need it.

I'll mark this PR as Ready for review once I'll have it tested on a microcontroller.

Signed-off-by: Błażej Sowa <[email protected]>
Signed-off-by: Błażej Sowa <[email protected]>
Signed-off-by: Błażej Sowa <[email protected]>
@bjsowa bjsowa marked this pull request as ready for review August 30, 2023 14:23
@bjsowa bjsowa requested a review from cguimaraes August 30, 2023 14:24
@bjsowa
Copy link
Contributor Author

bjsowa commented Sep 21, 2023

I updated the PR. @p-avital Could you rerun the workflows?

@Mallets
Copy link
Member

Mallets commented Oct 26, 2023

@bjsowa I've just realised that this PR is still open. In the meanwhile we have released zenoh 0.10.0 and it seems there are some conflicts to be resolved before merging.

@bjsowa
Copy link
Contributor Author

bjsowa commented Oct 26, 2023

@bjsowa I've just realised that this PR is still open. In the meanwhile we have released zenoh 0.10.0 and it seems there are some conflicts to be resolved before merging.

@Mallets I resolved the conflicts.

Copy link
Member

@cguimaraes cguimaraes left a comment

Choose a reason for hiding this comment

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

My previous comments are addressed

@Mallets Mallets merged commit 2ee5282 into eclipse-zenoh:master Oct 30, 2023
28 checks passed
@bjsowa bjsowa deleted the freertos branch October 30, 2023 10:11
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