Adapt to attached sensor independently of device_family value #27
Conversation
…er product string
| else if (parameter.get_name() == "packet_type") | ||
| { | ||
| std::string packet_type = parameter.as_string(); | ||
| if (packet_type == "A" || packet_type == "B" || packet_type == "C" || packet_type == "C1") |
There was a problem hiding this comment.
Is it may be necessary to check which packet types a device supports? For example, if an R2000 is connected and C1 arrives here, even though packet type C1 is only supported by the R2300.
There was a problem hiding this comment.
PFSDP does not provide information about supported values or ranges. "Checking what a device supports" would require hardcoded knowledge in the driver. It is the purpose of the changes in this branch to eliminate this requirement wherever possible. I'd rather try to improve the forwarding of existing verbose sensor error messages (e.g. "unsupported packet type") to the user instead of duplicating the checks in the driver.
There was a problem hiding this comment.
Maybe "Base" should be removed in the name of PFSDPBase (filename, classname) as it is no longer a base class (pfsdp_2000.cpp/h and pfsdp_2300.cpp/h have been removed)
There was a problem hiding this comment.
Yes, however, it's not wrong just because there are no derived classes. It the implementation of "base" PFSDP functionality anyway.. I'm afraid that maybe derivatives will be needed again at some time in the future
| @@ -8,6 +8,8 @@ struct ScanParameters | |||
| bool apply_correction = true; | |||
| int sampling_rate_max = 252000; | |||
| int scan_time_factor = 1; | |||
| int layer_count = 1; | |||
| int inclination_count = 1; | |||
There was a problem hiding this comment.
Is inclination_count really needed? As I understand it, layer_count and inclination_count are always 4 on a R2300. Maybe inclination_count can be omitted?
There was a problem hiding this comment.
inclination_count is just a local variable that indicates whether all layer_inclination values are equal (inclination_count=1) or not (inclination_count==layer_count), to decide whether a (3D) PointCloud topic should be published or just (2D) LaserScans. Previously, that decision was made based on device_family value.
I consider this a temporary solution to provide similar behaviour as before, but suggest to rather make PointCloud output user-configurable independently of all device_family, layer_count etc.
With these changes, the driver can adapt to the attached sensor product variant without making assumptions based just on some numeric value of
device_family. It allows to talk to future variants with new, yet unknowndevice_familyvalues, as long as they support the same commands.PointCloud and
/r2300_headeroutput for R2400-4-layer is enabled based on the presence and value of sensor'slayer_countand nonzerolayer_inclinationparameters. This may be made a user choice in the future. It was based ondevice_familyvalue before.This implements and fixes #22