ros2 / rmw_zenoh

RMW for ROS 2 using Zenoh as the middleware
Apache License 2.0
184 stars 34 forks source link

print router id on startup #189

Closed berndpfrommer closed 3 months ago

berndpfrommer commented 4 months ago

Minimal PR related to #188 This was enough for me to realize that it would take too long to ramp up (learning the basics of rust, loaning etc) to do a better job here. Pulling in the liveliness utils to print out the router id is ugly as well (but still beats code replication). I agree with @Yadunund that having the router ids of connected routers logged would be much better, but this is the best I can do in a limited amount of time. Please close if you plan on doing it right in the near future. Your work on rmw_zenoh is much appreciated.

JEnoch commented 4 months ago

Right, z_info_peers_zid() returns the connected peers' ids at the time of call. You could call it periodically to log changes, but that requires to maintain a state and compare at each call.

An alternative is to activate Zenoh logs via the RUST_LOG environment variable, that could be set via an option before Session creation in main.cpp:

Side note: reviewing this PR I saw that rmw_zenoh_cpp::liveliness::zid_to_str returns a hexadecimal string assuming zid is a big endian u128. Actually in Zenoh it's by convention a little endian u128. https://github.com/ros2/rmw_zenoh/pull/190 fixes this.

Yadunund commented 4 months ago

Thanks for the information @JEnoch!

An alternative is to activate Zenoh logs via the RUST_LOG environment variable, that could be set via an option before Session creation in main.cpp: RUST_LOG=zenoh=info for Session initialisation logs (i.e. id and listening endpoints) RUST_LOG=zenoh=info,zenoh_transport=debug to add connectivity events logging

^ I think it's definitely worth adding these to the README for users who want more details.

JEnoch commented 4 months ago

Done via https://github.com/ros2/rmw_zenoh/pull/191

@Yadunund do you think it's good enough, or shall we add to the router a --debug option that automatically set RUST_LOG in main.cpp (if not already defined) ? Or better, a cumulative -v option setting RUST_LOG to different values ?

berndpfrommer commented 4 months ago

I think adding the -v option would be a good idea. The additional documentation in #191 is also most welcome. I'd be happy to see this PR closed as obsolete.

Yadunund commented 4 months ago

Adding the -v flag as highlighted above sounds great. I'm still good with merging this PR in the meanwhile.

JEnoch commented 3 months ago

197 adds the -v to the router.

I think it's still worth merging this PR which prints the router id without -v. Using -v will also print it, but along other Zenoh startup logs (listening endpoints).