Make max_obstacle_height a dynamic parameter

[doisyg]

I need max_obstacle_height to be a dynamic parameter for my use case. Here is a PR reflecting how I do it (tested on a real robot). If the methodology seems acceptable, I am open to make this PR evolve to make more parameters dynamic. TBD which ones we should open.

[SteveMacenski]

The shared pointer mechanics are confusing and not really like anything else in the software. Further, it would be the only dynamic parameter enabled which is odd. The _max_obstacle_heights vector part is also hacky looking.

Overall, I'm not sure there's a path forward for this one to be merged back into STVL. There may be a less hacky way through this, but at this moment, I'm not seeing it without other compromises.

[doisyg]

Further, it would be the only dynamic parameter enabled which is odd

My idea with this PR was to discuss what mechanism to use starting with max_obstacle_heights before generalizing it to other parameters that could benefit from being dynamic.

[nickvaras]

This was in part the justification for an earlier PR that aimed at bypassing the forced built-in filtering (passthtrough or voxel). Then you can dynamically adjust those filters to effectively change the max_obstacle_height on the fly.

[SteveMacenski]

What all parameters do you plan on making dynamic? The subset matters as to other potential strategies. If you want to change things like the actual nature of the buffer (add/remove clearing/marking) that throws a wrench into things. If its just attributes of data handling, that could be worked around.

For instance, making a new function in the MeasurementBuffer for changing min/max heights that can be called in dynamic parameter callbacks. Then there's none of this shared pointer stuff, nor the requirement for the vector. Just a new method in the buffer class that the dynamic parameter callback calls to set some variables. Seems much more straight forward :wink:

[doisyg]

For instance, making a new function in the MeasurementBuffer for changing min/max heights that can be called in dynamic parameter callbacks. Then there's none of this shared pointer stuff, nor the requirement for the vector. Just a new method in the buffer class that the dynamic parameter callback calls to set some variables. Seems much more straight forward wink

Yes, I like your suggestion of having a dedicated function for setting the MeasurementBuffer max_obstacle_heightsand other parameters following the same logic. At the minimum:

• min_obstacle_height
• max_obstacle_height

=> void SetMinMaxObstacleHeight(const double & min_obstacle_height, const double & min_obstacle_height)

I don't use them but maybe for completeness also:

• min_z
• max_z
• vertical_fov_angle
• vertical_fov_padding
• horizontal_fov_angle

=> void SetMinMaxZ(const double & min_z, const double & min_z) => void SetFovs(const double & vertical_fov_angle, const double & vertical_fov_padding, const double & horizontal_fov_angle) Or all in the same SetDynParam function ?

Also, there is already a SetEnabled function exposed in a service callback, I would like to use it to make enabled dynamic.

Finally, maybe we could add a field source_name in MeasurementBuffer in order to iter on _observation_buffers more safely.

[SteveMacenski]

That all sounds good to me.

It should all be in the same parameter callback, but only need to call the appropriate functions for what parameters were actually changed. Having the APIs as you suggest them here could be problematic since each parameter update is a single parameter (then looping over N of them) so when you SetFOVs for instance, you're really only updating 1 at a time. What are the values for the others?

Might be worth setting each independently in a function, or having a way to specify which are changed

[doisyg]

each parameter update is a single parameter (then looping over N of them) so when you SetFOVs for instance, you're really only updating 1 at a time

Hum, yes, that seems quite inefficient and feels wrong

What are the values for the others?

The other values could be taken from the parameter namespace using the changed parameter source. But that doesn't change the issue of having to loop over N of them

One alternative is to make the MeasurementBuffer global variables in question public to directly access them in the callback but that really feels like bad practice

Might be worth setting each independently in a function,

Looks like it is the most decent alternative, though I am not a fan of creating multiple functions

or having a way to specify which are changed

Don't see how to do that properly

Anyway, I went ahead with the multiple functions way, took me less time than writing this. PR update is ready, I ll push once tested

[doisyg]

Tested and updated

[SteveMacenski]

Thanks! As always! Great to have the help

[ghost]

Cool feature. Any chance this gets backported to foxy-devel?

[SteveMacenski]

If you submit a PR over, I'll merge it and run bloom!