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 base station rtcm message publishing support #30

Merged
merged 9 commits into from
Dec 13, 2024

Conversation

bvsam
Copy link
Contributor

@bvsam bvsam commented Dec 12, 2024

Hello.

This PR adds in the capability to publish RTCM3 messages being emitted by a GPS, supporting the use case where one has a fixed base station they're using to publish RTCM messages which are then received by a moving rover.

I tried to keep my changes to the code minimal and it mostly follows the existing structure of the code.

I've tested my changes with 2 ZED-F9Ps on the same ROS network, and it works. The base station can publish RTCM3 messages and the rover can successfully subscribe to the RTCM topic and use these messages, as confirmed by the /rover/ubx_rxm_rtcm topic (where msg_used is 2, indicating the RTCM messages are being used).

Contributing this PR so that people can use this package in a fixed base station use case. Please let me know if you'd like me to make any changes.

@hortovanyi
Copy link
Contributor

Looks good. Is it possible to create a launch file for the base station as opposed to the rover? ... we need to also update the readme I suspect so people can work out how to set this scenario up?

@hortovanyi hortovanyi self-requested a review December 13, 2024 05:12
@hortovanyi hortovanyi merged commit 125b3ae into aussierobots:main Dec 13, 2024
@bvsam
Copy link
Contributor Author

bvsam commented Dec 13, 2024

Looks good. Is it possible to create a launch file for the base station as opposed to the rover? ... we need to also update the readme I suspect so people can work out how to set this scenario up?

Hi. I only saw this now, after you merged this in. I can definitely do a basic README update describing this use case in a new PR if you'd like.

In terms of the launch files for this use case, I was thinking the existing ublox_rover_hpposllh_navsatfix.launch.py launch file should cover the nodes require to run the rover, but I can update it to add a launch parameter to set the namespace of the 2 nodes being run (so that one could set it to /rover, for example) and also add a launch parameter to set the DEVICE_SERIAL_STRING parameter passed into the ublox_dgnss_node being run.

When it comes to the base's launch file, I was thinking that it can run the ublox_dgnss_node under a base namespace (the namespace name can be a launch param) and I can also add in a launch param for the DEVICE_SERIAL_STRING being used. When it comes to piping in the RTCM messages to the rover, I could remap the base's rtcm topic to /ntrip_client/rtcm. This is what I've been testing with and it's convenient since one can easily switch between piping in RTCM messages from an NTRIP client or from a base station without having to stop the rover's ublox_dgnss_node and remap its topics (even though it might be confusing to have RTCM messages being published on /ntrip_client/rtcm without even having an NTRIP client running). Alternatively, to prevent the possible confusion of publishing non-NTRIP messages on the /ntrip_client/rtcm topic, I can modify the ublox_dgnss_node code to subscribe to a rtcm_in topic and execute the same callback used for messages received on the /ntrip_client/rtcm topic, then ask the base to remap it's rtcm topic to /rover/rtcm_in.

Alternatively, the base could just publish on /base/rtcm and a parameter could be added to the ublox_dgnss_node code to specify which topic to subscribe to RTCM messages from (with /ntrip_client/rtcm as its default value).

There are a few different ways of going about adding launch files for this use case, what are your thoughts? I can add in the README changes and the new launch files in a new PR once it's clear what to add.

@bvsam
Copy link
Contributor Author

bvsam commented Dec 13, 2024

Also, unfortunately, I don't think the base station's launch file can really do much other than startup a ublox_dgnss_node and remap the topics as necessary. It can't actually configure the GPS to act as a base station with ublox VALSET commands, since from what I understand, the current code (at ublox_dgnss_node/include/ublox_dgnss_node/ubx/ubx_cfg_item.hpp) doesn't currently support setting the values of keys such as CFG-MSGOUT-RTCM_3X_TYPE1005_ UART1 (and others) which are required for the GPS to actually output RTCM messages. The launch file can only assume that the person running it has already configured their GPS properly for this use case.

I could add in the ability to set values for keys like CFG-MSGOUT-RTCM_3X_TYPE1005_UART1 and others that aren't currently supported (all of the relevant ones required to setup the GPS as a fixed base station described in Appendix Section C of the ZED-F9P's integration manual would ideally have to be supported) so that the launch file can actually configure the GPS with VALSET commands to act as a base station emitting RTCM messages. But this might take a while. Let me know if you think this would be useful and I might be able to add it into a PR with the README and launch file updates.

@hortovanyi
Copy link
Contributor

I think we should add new launch files for these scenarios - especially the base station. I know others use the existing default ones and if we change them, then their environments may not work. It might suffice to just override the output from a base station launchfile to publish to '/ntrip_client/rtcm' topic??

Have a look also at the moving base station launch files.

Are all the configuration parameters there to configure the F9P base station to publish rtcm on usb? Think all the parameters from the moving base station have been added to ubx_cfg_item.hppp

In the launch files we should be able to override the topics that are subscribed to, published to.. cant remember how to do it off the top of my head.

Yes I merged the PR and made sure it all built etc .. a couple of uncrustify formatting issues. That always happens.

@hortovanyi
Copy link
Contributor

Also, unfortunately, I don't think the base station's launch file can really do much other than startup a ublox_dgnss_node and remap the topics as necessary. It can't actually configure the GPS to act as a base station with ublox VALSET commands, since from what I understand, the current code (at ublox_dgnss_node/include/ublox_dgnss_node/ubx/ubx_cfg_item.hpp) doesn't currently support setting the values of keys such as CFG-MSGOUT-RTCM_3X_TYPE1005_ UART1 (and others) which are required for the GPS to actually output RTCM messages. The launch file can only assume that the person running it has already configured their GPS properly for this use case.

I could add in the ability to set values for keys like CFG-MSGOUT-RTCM_3X_TYPE1005_UART1 and others that aren't currently supported (all of the relevant ones required to setup the GPS as a fixed base station described in Appendix Section C of the ZED-F9P's integration manual would ideally have to be supported) so that the launch file can actually configure the GPS with VALSET commands to act as a base station emitting RTCM messages. But this might take a while. Let me know if you think this would be useful and I might be able to add it into a PR with the README and launch file updates.

might need to add these ... but to work on USB. I dont have two F9Ps so not sure which ones and cant test it all out

@hortovanyi
Copy link
Contributor

There are a lot of parameters slowly being added. At some stage I was going to refactor how this works such that a TOML file, that could be referenced in a launch file contained the cfg items to use.... might be something to do next year

@hortovanyi
Copy link
Contributor

@gsokoll did you add all the parameters into ubx_cfg_item.hpp for the moving base station etc?

@bvsam
Copy link
Contributor Author

bvsam commented Dec 13, 2024

I was checking that right now and it seems like it hasn't been added. This is some of the output I get when running the moving base launch file:

[component_container_mt-1] [INFO] [1734076815.046430409] [base.ublox_dgnss]: Parameter FRAME_ID found with value: base
[component_container_mt-1] [INFO] [1734076815.046507867] [base.ublox_dgnss]: parameter supplied: CFG-MSGOUT-RTCM_3X_TYPE1005_UART2
[component_container_mt-1] [WARN] [1734076815.046603297] [base.ublox_dgnss]: parameter supplied: CFG-MSGOUT-RTCM_3X_TYPE1005_UART2 is not recognised. Ignoring!
[component_container_mt-1] [INFO] [1734076815.046641781] [base.ublox_dgnss]: parameter supplied: CFG-MSGOUT-RTCM_3X_TYPE1074_UART2
[component_container_mt-1] [WARN] [1734076815.046673583] [base.ublox_dgnss]: parameter supplied: CFG-MSGOUT-RTCM_3X_TYPE1074_UART2 is not recognised. Ignoring!
[component_container_mt-1] [INFO] [1734076815.046697756] [base.ublox_dgnss]: parameter supplied: CFG-MSGOUT-RTCM_3X_TYPE1084_UART2
[INFO] [launch_ros.actions.load_composable_nodes]: Loaded node '/base/ublox_nav_sat_fix_hp' in container '/ublox_nav_sat_fix_hp_container'
[component_container_mt-1] [WARN] [1734076815.046718171] [base.ublox_dgnss]: parameter supplied: CFG-MSGOUT-RTCM_3X_TYPE1084_UART2 is not recognised. Ignoring!
[component_container_mt-1] [INFO] [1734076815.046738278] [base.ublox_dgnss]: parameter supplied: CFG-MSGOUT-RTCM_3X_TYPE1124_UART2

@hortovanyi
Copy link
Contributor

@bvsam think @gsokoll has them coded up somewhere :) ... am assuming in your scenario the two F9Ps are on the same network but over a distance

@bvsam
Copy link
Contributor Author

bvsam commented Dec 13, 2024

Ok so I can get a new PR going with 2 new launch files and a README update for this use case.

The rover's one will essentially be a copy of the ublox_rover_hpposllh_navsatfix.launch.py but with 2 launch parameters added to set the namespace and DEVICE_SERIAL_STRING. (Just to note, I do think modifying ublox_rover_hpposllh_navsatfix.launch.py with 2 new launch parameters wouldn't mess up the environments of people who use this package since parameters are only being added (with non-breaking defaults), not being removed/changed... let me know if you're fine with just using the existing ublox_rover_hpposllh_navsatfix.launch.py for the rover instead of creating a new launch file that's mostly a copy.)

Then for the base station I can add in the VALSET keys to ubx_cfg_item.hpp for the base station to be configured for this use case. Then the base station launch file will run ublox_dgnss_node and pass in the relevant VALSET parameters to configure the GPS, while also having the same 2 launch params as the rover's launch file.

@bvsam
Copy link
Contributor Author

bvsam commented Dec 13, 2024

@bvsam think @gsokoll has them coded up somewhere :) ... am assuming in your scenario the two F9Ps are on the same network but over a distance

Good to hear it's there somewhere. Although, I can add the keys in the PR I'm planning to make for the launch files and the VALSET keys for my use case if you'd like.

Also, you're correct about what we're doing in our scenario.

@hortovanyi
Copy link
Contributor

@bvsam if you can that would be great (I think Geoff is working on another aspect of his project presently).. if the default behaviour of the launch files is unchanged, then go with what seems best for you.

Sometimes I think keeping the launch files simple for a specific purpose is better. The original ones I put up were for what I needed. Thought they'd make good examples for others to use.

Reading between the lines, am assuming with your Use Case you are looking ultimately to have a High Precision NavSatFix message using your own base station.

@hortovanyi
Copy link
Contributor

@bvsam ohh the other thing to look at, if it interests you, is that I have basically set it up so that the QoS can be overrided in the launch files ... #29

@bvsam
Copy link
Contributor Author

bvsam commented Dec 13, 2024

You're correct about us trying to get a high precision NavSatFix message working with our own base station.

I'm also a bit confused: what does the QoS overriding have to do with this use case?

@hortovanyi
Copy link
Contributor

hortovanyi commented Dec 13, 2024

I'm also a bit confused: what does the QoS overriding have to do with this use case?

I noticed you changed the QoS on the RTCM publisher from SensorQoS rclcpp::QoS(10) which I think is the system default

I just re-read the link in the issue linked earlier. It recommends for Publishers to use the QoS system default and for others consuming sensor data to use SensorQoS.

@bvsam
Copy link
Contributor Author

bvsam commented Dec 13, 2024

I'm also a bit confused: what does the QoS overriding have to do with this use case?

I noticed you changed the QoS on the RTCM publisher from SensorQoS rclcpp::QoS(10) which I think is the system default

I just re-read the link in the issue linked earlier. It recommends for Publishers to use the QoS system default and for others consuming sensor data to use SensorQoS.

It's true that I did that. I did so to ensure compatibility with the current codebase and to not mess things up for users of this package. I initially set the publisher on the rtcm topic to use SensorQoS like the other publishers but it turned out this was incompatible when piping in the base's RTCM messages to the rover, since the rover's /ntrip_client/rtcm subscription uses non-SensorQoS and so was incompatible with a SensorQoS publisher.

Are you suggesting that the launch files that I plan to make should ensure that the recommended SystemDefaultQoS for publishers and SensorDataQoS for subscriptions is being used? If so, I can add this in (assuming it's possible - haven't tried it out myself yet).

@hortovanyi
Copy link
Contributor

This is something that eventually needs to be resolved - I'm leaning towards just changing everything and then saying in the readme this is how you can override it.

I originally published everything as SensorQoS as that is what I thought made sense. However it causes problems eg ros bags you need to override qos to record.

I think for what you are doing publishers should use the default QoS ...

@bvsam bvsam deleted the base-station-rtcm branch December 14, 2024 22:22
@gsokoll
Copy link
Contributor

gsokoll commented Dec 16, 2024

@bvsam @hortovanyi sorry, I haven't as yet added the ability to configure the msg outputs as required for base + rover operation. As suspected, we've previously just flashed required configs to each device for either base or rover roles.

@bvsam If you've already started on adding in the required messages, I'm happy for you to continue. But I can probably do so later this week otherwise.

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