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

Use a zenoh session to run the router #101

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

clalancette
Copy link
Collaborator

The main benefit here is that we stop vendoring zenohd, which should improve our compile times. While in here, we also rewrite the keyboard handling so it will work on both Linux and Windows (though only tested on Linux so far).

This is mostly the work of @francocipollone ; I just did the keyboard handling part. This should fix #64

francocipollone and others added 2 commits January 23, 2024 11:48
This eliminates the need for zenohd_vendor,
which we also remove here.

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
This should work for both Linux and Windows, though only
tested on Linux so far.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette clalancette requested a review from Yadunund January 24, 2024 14:55
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

The code here works great. Did we come to a conclusion regarding tradeoffs between rust vs C based impl as discussed here?

If we want to stick to the C approach, I think it would be nice to refactor detail/zenoh_config.hpp to be flexible enough to return configs for sessions or routers. Perhaps even look for an envar for the router config as well.

@clalancette
Copy link
Collaborator Author

The code here works great. Did we come to a conclusion regarding tradeoffs between rust vs C based impl as discussed here?

As of right now, I don't think we need any router features that we can't get via the configuration file. Given how far along we are in the implementation, it seems to me that we probably won't need it; the one possible exception is security, which we haven't thought about at all. The good news is that if we merge this, then we have both options in the history; we have the ability to run the Rust-based one (the commits prior to this), and we'll have this one. So worst case, we could always revert this and go back to the Rust-based router.

If we want to stick to the C approach, I think it would be nice to refactor detail/zenoh_config.hpp to be flexible enough to return configs for sessions or routers. Perhaps even look for an envar for the router config as well.

Yeah, that would work. That way the user could also configure the router, which is one of our long-term goals anyway. I'll take a look at doing that.

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying. Happy to merge first and work on reusing the config generator code in a separate PR. Maybe we can open a ticket so we don't forget.

@clalancette
Copy link
Collaborator Author

Thanks for clarifying. Happy to merge first and work on reusing the config generator code in a separate PR. Maybe we can open a ticket so we don't forget.

Yep, good call. I've opened up #102 to track that.

@clalancette clalancette merged commit f2a1432 into rolling Jan 25, 2024
5 checks passed
@clalancette clalancette deleted the francocipollone/router_via_zenoh_session branch January 25, 2024 16:33
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.

Vendor zenohd
3 participants