broadcaster for magnetic field values from a magnetometer#2214
broadcaster for magnetic field values from a magnetometer#2214christianrauch wants to merge 4 commits intoros-controls:masterfrom
Conversation
|
Hey @christianrauch, I see its some usage. Let's see what maintainers have to say. And another thing, why is the class declaration is in implementation file (everything is in .cpp file). It'll be really better if declaration was in .hpp and implementation was in .cpp |
9d9b9fc to
22c1d40
Compare
There was a problem hiding this comment.
The hpp/cpp comment is a good catch indeed but since this is delivered as a plugin I'm not too concerned about it. Thanks for pointing it out @thedevmystic
22c1d40 to
cddcb89
Compare
|
I addressed the one comment about the maintainers and fixed one CI issue from my side. The CI shows a couple of other issues, such as missing dependencies on |
cddcb89 to
45bea18
Compare
christophfroehlich
left a comment
There was a problem hiding this comment.
I haven't reviewed the code yet, but there is a lack of documentation. At least a brief description rst file, added to the controller_index and a note in the release_notes is mandatory.
45bea18 to
b5c6ffa
Compare
b5c6ffa to
7876ec6
Compare
I removed the FYI, I essentially took them from the ros2_controllers/imu_sensor_broadcaster/package.xml Lines 37 to 38 in fc570f8 If they are not required for the packages in this repo, you also want to remove them from there. I will add the documentation later, but I would like to see that the CI is happy first. |
|
7876ec6 to
414ff43
Compare
I also added the documentation now. Thanks for the feedback. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2214 +/- ##
==========================================
+ Coverage 84.68% 84.69% +0.01%
==========================================
Files 153 156 +3
Lines 15319 15430 +111
Branches 1332 1333 +1
==========================================
+ Hits 12973 13069 +96
- Misses 1858 1872 +14
- Partials 488 489 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This PR adds a broadcaster for magnetometers via the
semantic_components::MagneticFieldSensor, similar to how this is done for other semantic components (semantic_components::IMUSensor,semantic_components::ForceTorqueSensor).colcon testandpre-commit run(requires you to install pre-commit bypip3 install pre-commit)