Skip to content

Add GPSBroadcaster#1554

Merged
christophfroehlich merged 5 commits intoros-controls:masterfrom
Wiktor-99:gps_broadcaster
Apr 5, 2025
Merged

Add GPSBroadcaster#1554
christophfroehlich merged 5 commits intoros-controls:masterfrom
Wiktor-99:gps_broadcaster

Conversation

@Wiktor-99
Copy link
Copy Markdown
Contributor

As in the title. I'm open to any suggestions.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 23, 2025

Codecov Report

Attention: Patch coverage is 86.09272% with 21 lines in your changes missing coverage. Please review.

Project coverage is 85.17%. Comparing base (9ed7cbe) to head (4863d59).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
..._sensor_broadcaster/src/gps_sensor_broadcaster.cpp 71.23% 21 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1554      +/-   ##
==========================================
+ Coverage   85.16%   85.17%   +0.01%     
==========================================
  Files         123      126       +3     
  Lines       11755    11907     +152     
  Branches      996      999       +3     
==========================================
+ Hits        10011    10142     +131     
- Misses       1431     1451      +20     
- Partials      313      314       +1     
Flag Coverage Δ
unittests 85.17% <86.09%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...r_broadcaster/test/test_gps_sensor_broadcaster.cpp 100.00% <100.00%> (ø)
...adcaster/test/test_load_gps_sensor_broadcaster.cpp 100.00% <100.00%> (ø)
..._sensor_broadcaster/src/gps_sensor_broadcaster.cpp 71.23% <71.23%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Thank you for this new controller. I've found some on quick look
I'll send more reviews during this week.

@Timple
Copy link
Copy Markdown
Contributor

Timple commented Feb 24, 2025

Conceptual question:

We also have battery_state_broadcaster. Now we get a gps_broadcaster.

Now I can imagine we get a lot of future PR's creating packages for all items in this list: sensor_msgs.

Would it make sense to template this such that we can simply send any message from sensor_msgs?

@Wiktor-99
Copy link
Copy Markdown
Contributor Author

@Timple Perhaps you're right; it could be a template class. However, since all sensors would use a different set of state interfaces, we would probably need to create a bunch of new semantic components (which can be done).

Also, it looks like this decision was already settled. We've got pose/IMU/range broadcasters, and templating them would affect existing configs.

@Wiktor-99 Wiktor-99 requested a review from saikishor February 26, 2025 11:06
Copy link
Copy Markdown
Member

@christophfroehlich christophfroehlich 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 the follow-up to the semantic component. But I have some comments

btw: To fix the docs workflow, please add the userdoc.rst here.

Copy link
Copy Markdown
Member

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

LGTM

@christophfroehlich christophfroehlich merged commit d1349e1 into ros-controls:master Apr 5, 2025
24 of 28 checks passed
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.

4 participants