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 parameter to change azimuth window #98

Closed

Conversation

bertaveira
Copy link
Contributor

I don't think I am the only one using this configuration of the azimuth window. Previously we would set it through the web configurator and then it would be saved but on recent firmware versions it sometimes resets to 0-360 azimuth window which is unfortunate.

Therefore I made so that there is a parameter and the ROS driver sets this parameter as well.

@bertaveira
Copy link
Contributor Author

tested on OS1-64 FW 2.2.1 on ROS foxy

@SteveMacenski
Copy link
Member

Any modifications to the client code needs to be merged into the ouster example library first before here, that way they remain in sync and when updates are pushed there, they can be easily migrated here. Please submit the client-modifying code there and have it merged first, then reopen here and we can make quick work of it.

@@ -419,6 +419,15 @@ optional<NMEABaudRate> nmea_baud_rate_of_string(const std::string& s)
return res == end ? nullopt : make_optional<NMEABaudRate>(res->first);
}

optional<AzimuthWindow> azimuth_window_of_string(const std::string& s)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SteveMacenski How should I do here then? I see that this file is very different here in this repo compared to the example client.

This simillar function is quite different in here: https://github.com/ouster-lidar/ouster_example/blob/13ea8e8b8a4951fb630dbc9108666995c8443bf6/ouster_client/src/types.cpp#L325

Is there a reason for it? It seems that the ROS implementation uses a different way of formatting certain configs

Copy link
Member

Choose a reason for hiding this comment

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

You're looking at an outdated version from search, use the branch mechanism https://github.com/ouster-lidar/ouster_example/blob/master/ouster_client/src/types.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You linked the exact same commit but in "master". The file seems very different from what is here. Functionally very similar but the use of a new function "rlookup()" for most of the string conversions as well as code organization. I am not sure how to go about it.

I am not sure that copying that more recent "client" and "types" .cpp and .hpp files would work here. I can change there and then change here similarly but keeping the same differences between the files.

Copy link
Member

Choose a reason for hiding this comment

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

We linted the file probably for our formatting but it should be the same, if its not that's because they've made updates and we just haven't gotten to adding them here as well.

@bertaveira
Copy link
Contributor Author

PR waiting for approval on the ouster_example. Hopefully soon we can port it here
ouster-lidar/ouster-sdk#366

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.

2 participants