-
Notifications
You must be signed in to change notification settings - Fork 0
Rover 394 #30
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
base: main
Are you sure you want to change the base?
Rover 394 #30
Conversation
…ate, removed the old bad way of config
ConnorNeed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some things to get started with.
I will challenge you to see if you can create a helper python library that contains all the shared helper functions reducing code dupliations.
Given that the configuration scripts are going to be used as command line tools, use argparse (https://docs.python.org/3/library/argparse.html) to allow us to change some of the parameters from the command line. Make sure to use whatever the "Correct" versions for our setup as the defaults.
src/Nav/gps/gps/rtcm_pub_node.py
Outdated
| self.get_parameter("Baudrate").get_parameter_value().integer_value | ||
| ) | ||
| self.declare_parameter("Device", "/dev/ttyACM0") | ||
| self.declare_parameter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the default path. It can be overwriten in the launch file. Using /dev/ttyACM0 makes sense as a default.
src/Nav/gps/config/fr_basestation.py
Outdated
| """ | ||
|
|
||
| print("\nFormatting RTCM MSGOUT CFG-VALSET message...") | ||
| layers = 7 # 1 = RAM, 2 = BBR, 4 = Flash (can be OR'd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create some named constants to remove this magic number
src/Nav/gps/config/fr_basestation.py
Outdated
| "/dev/ttyACM0", | ||
| 38400, | ||
| ) as stream: | ||
| layers = 7 # RAM + BBR + Flash for persistent storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concern as above
src/Nav/gps/config/fr_heading.py
Outdated
| layers = 7 # RAM + BBR + Flash for persistent storage | ||
| transaction = 0 | ||
| cfg_data = [ | ||
| ("CFG_MSGOUT_UBX_NAV_RELPOSNED_USB", 1), # 1 = 1 time per epoch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the conversion from epoch to hz?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is 1/rate_ms, where the rate should be in seconds and NAV_RATE_MS, so a value of NAV_RATE_MS is 200 is equal to 5 hz
|
Before you start worring about the new task of reading the base station via a different interface this needs to get cleaned up and merged in. |
…did everything how you asked for it
ConnorNeed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better. Good work
| { | ||
| "Device": "/dev/serial/by-id/usb-FTDI_FT230X_Basic_UART_D30I1LY5-if00-port0" | ||
| }, | ||
| # expose launch arg to node; node can act on it to load config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was obviously AI but please double check the work. This doesn't make any sense in the context of the PR. The part right below it runs the config. Why are we passing it into the node?
| output="both", | ||
| remappings=ublox_remappings, | ||
| parameters=[params_file], | ||
| parameters=[params_file, {"load_config": load_config}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue
| ) | ||
|
|
||
| return launch.LaunchDescription([ublox_gps_node, heading_cmd]) | ||
| return launch.LaunchDescription( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is some of this declared above and some of this in the return statement? Either style is fine but please pick one
Remade the config files for the gps to factory reset before and changed the frequency of gps/fix and the heading