Add GPS semantic component #2000

Wiktor-99 · 2025-01-13T08:52:46Z

Adding initial version of GPSSensor semantic components. I decided to add two with covariance interface and w/o interface.

It'll be followed by GPS broadcaster in ros2_controllers.

I'm open too all suggestions.

codecov · 2025-01-13T09:07:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.34%. Comparing base (f8c03cc) to head (b4a1ee7).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2000      +/-   ##
==========================================
- Coverage   89.35%   89.34%   -0.01%     
==========================================
  Files         130      132       +2     
  Lines       14576    14666      +90     
  Branches     1258     1258              
==========================================
+ Hits        13024    13104      +80     
- Misses       1088     1094       +6     
- Partials      464      468       +4

Flag	Coverage Δ
unittests	`89.34% <100.00%> (-0.01%)`	⬇️

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

Files with missing lines	Coverage Δ
...terface/include/semantic_components/gps_sensor.hpp	`100.00% <100.00%> (ø)`
controller_interface/test/test_gps_sensor.cpp	`100.00% <100.00%> (ø)`

... and 4 files with indirect coverage changes

Maverobot · 2025-01-13T09:19:22Z

controller_interface/include/semantic_components/gps_sensor.hpp

+public:
+  explicit GPSSensorWithCovariance(const std::string & name) : GPSSensor(name)
+  {
+    interface_names_.emplace_back(name + "/" + "latitude_covariance");


A general question:

Does it make sense to store hard-coded strings in member variables to prevent users from duplicating those strings in their code? Or is this a intended design decision?

I think it was designed that way. It provides the user (e.g., the controller) with an interface ready to use. The member functions of the semantic components are tightly coupled with those strings.

saikishor

The tests look very good. Good job @Wiktor-99

controller_interface/include/semantic_components/gps_sensor.hpp

controller_interface/test/test_gps_sensor.cpp

controller_interface/include/semantic_components/gps_sensor.hpp

christophfroehlich

I never used GPS in the context of ROS. But having a look at the message definition, shouldn't we add also NavSatStatus to the component?

int8 status
uint16 service

I think this can be useful and every receiver has this as output, but is maybe depending on the used protocol like NMEA, ubx, ..

Wiktor-99 · 2025-01-13T22:09:56Z

The status is already there. I'm not sure about service. I don't know if it can be dynamically updated, I'll check it.

Wiktor-99 · 2025-01-14T20:03:37Z

@christophfroehlich I added service filling in the PR

controller_interface/include/semantic_components/gps_sensor.hpp

christophfroehlich

Thanks, LGTM!

saikishor

Thanks for going through all the changes. This is good work

LGTM

saikishor

LGTM

Maverobot · 2025-01-16T21:26:06Z

controller_interface/include/semantic_components/gps_sensor.hpp

+
+#include <array>
+#include <string>
+#include <vector>


#include <vector> seems to be unused.

Maverobot · 2025-01-16T21:27:11Z

controller_interface/test/test_gps_sensor.cpp

+#include "hardware_interface/handle.hpp"
+#include "hardware_interface/loaned_state_interface.hpp"
+#include "semantic_components/gps_sensor.hpp"
+#include "sensor_msgs/msg/nav_sat_fix.hpp"


#include <vector> should be added as std::vector is used in this file.

Maverobot · 2025-01-16T21:31:23Z

controller_interface/include/semantic_components/gps_sensor.hpp

+   *
+   * \return Status
+   */
+  int8_t get_status() const { return static_cast<int8_t>(state_interfaces_[0].get().get_value()); }


It would IMHO make sense to add [[nodiscard]] for all those getters.

You're right but then I believe we should add [[nodiscard]] to each semantic component's getters.
Maybe I'll align it the next PR, what do you think?

Sure. Sounds good.

Maverobot · 2025-01-16T21:37:44Z

controller_interface/include/semantic_components/gps_sensor.hpp

+namespace semantic_components
+{
+
+template <bool use_covariance>


I'm not vary familiar with GPS Sensor in general. Are both cases equally often used? If one of them is used more often, you may consider giving use_covariance a default value.

It really depends, I would leave it as it is.

I agree with @Wiktor-99, It is better to leave it as it is. This way the user is conscious about the type is choosing

Fine by me.

Maverobot · 2025-01-17T08:41:15Z

controller_interface/include/semantic_components/gps_sensor.hpp

+namespace semantic_components
+{
+
+template <bool use_covariance>


[minor] GPSSensor<true> or GPSSensor<false> is not very good in terms of readability for me personally. You can choose use enum class instead of bool to allow usages like:

GPSSensor<GPSSensorOption::kWithCovariance> and GPSSensor<GPSSensorOption::kWithoutCovariance>

Feel free to use better class and enum names.

Add GPS semantic component.

1a02b2e

github-actions bot requested review from aprotyas, christophfroehlich, DasRoteSkelett, destogl, fmauch, Maverobot and moriarty January 13, 2025 08:53

Maverobot reviewed Jan 13, 2025

View reviewed changes

saikishor reviewed Jan 13, 2025

View reviewed changes

aprotyas reviewed Jan 13, 2025

View reviewed changes

controller_interface/include/semantic_components/gps_sensor.hpp Outdated Show resolved Hide resolved

christophfroehlich reviewed Jan 13, 2025

View reviewed changes

Wiktor-99 added 3 commits January 14, 2025 09:00

Merge GPSSensor classes in to one

16da7e9

Add service interface

f8d0edc

Fix casting

d638af7

saikishor reviewed Jan 14, 2025

View reviewed changes

controller_interface/include/semantic_components/gps_sensor.hpp Outdated Show resolved Hide resolved

christophfroehlich previously approved these changes Jan 14, 2025

View reviewed changes

saikishor previously approved these changes Jan 14, 2025

View reviewed changes

Make covariance always available

165c3c3

Wiktor-99 dismissed stale reviews from saikishor and christophfroehlich via 165c3c3 January 14, 2025 21:15

Merge branch 'master' into gps_semantic_component_interface

7d67159

saikishor approved these changes Jan 14, 2025

View reviewed changes

Merge branch 'master' into gps_semantic_component_interface

b4a1ee7

christophfroehlich requested review from Maverobot and aprotyas January 16, 2025 21:22

christophfroehlich approved these changes Jan 16, 2025

View reviewed changes

Maverobot reviewed Jan 16, 2025

View reviewed changes

Maverobot reviewed Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GPS semantic component #2000

Add GPS semantic component #2000

Wiktor-99 commented Jan 13, 2025

codecov bot commented Jan 13, 2025 •

edited

Loading

Maverobot Jan 13, 2025 •

edited

Loading

Wiktor-99 Jan 13, 2025

saikishor left a comment

christophfroehlich left a comment

Wiktor-99 commented Jan 13, 2025

Wiktor-99 commented Jan 14, 2025

christophfroehlich left a comment

saikishor left a comment

saikishor left a comment

Maverobot Jan 16, 2025 •

edited

Loading

Maverobot Jan 16, 2025 •

edited

Loading

Maverobot Jan 16, 2025

Wiktor-99 Jan 17, 2025

Maverobot Jan 17, 2025

Maverobot Jan 16, 2025 •

edited

Loading

Wiktor-99 Jan 17, 2025

saikishor Jan 17, 2025

Maverobot Jan 17, 2025

Maverobot Jan 17, 2025 •

edited

Loading

Add GPS semantic component #2000

Are you sure you want to change the base?

Add GPS semantic component #2000

Conversation

Wiktor-99 commented Jan 13, 2025

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

Maverobot Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Wiktor-99 Jan 13, 2025

Choose a reason for hiding this comment

saikishor left a comment

Choose a reason for hiding this comment

christophfroehlich left a comment

Choose a reason for hiding this comment

Wiktor-99 commented Jan 13, 2025

Wiktor-99 commented Jan 14, 2025

christophfroehlich left a comment

Choose a reason for hiding this comment

saikishor left a comment

Choose a reason for hiding this comment

saikishor left a comment

Choose a reason for hiding this comment

Maverobot Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Maverobot Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Maverobot Jan 16, 2025

Choose a reason for hiding this comment

Wiktor-99 Jan 17, 2025

Choose a reason for hiding this comment

Maverobot Jan 17, 2025

Choose a reason for hiding this comment

Maverobot Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Wiktor-99 Jan 17, 2025

Choose a reason for hiding this comment

saikishor Jan 17, 2025

Choose a reason for hiding this comment

Maverobot Jan 17, 2025

Choose a reason for hiding this comment

Maverobot Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 13, 2025 •

edited

Loading

Maverobot Jan 13, 2025 •

edited

Loading

Maverobot Jan 16, 2025 •

edited

Loading

Maverobot Jan 16, 2025 •

edited

Loading

Maverobot Jan 16, 2025 •

edited

Loading

Maverobot Jan 17, 2025 •

edited

Loading