AC_Avoidance: correct use of uninitialised value when retrying fence …

[peterbarker]

…creation

this shortcut (fences haven't changed, so don't bother retrying) wasn't setting the passing-in error ID, leading to use of unininitialised stack data.

So remember the error ID and return it in this case

[magicrub]

identiacally-named functions like this a ripe for confusion

[peterbarker]

On Sun, 21 Jan 2024, Tom Pittenger wrote: In libraries/AC_Avoidance/AP_OADijkstra.h: > @@ -76,6 +76,10 @@ class AP_OADijkstra { // create polygons inside the existing inclusion polygons // returns true on success. returns false on failure and err_id is updated bool create_inclusion_polygon_with_margin(float margin_cm, AP_OADijkstra_Error &err_id); + // create polygons inside the existing inclusion polygons + // returns true on success. returns false on failure and err_id is updated. This function is wrapped so we can remember the error ID. + bool _create_inclusion_polygon_with_margin(float margin_cm, AP_OADijkstra_Error &err_id); identiacally-named functions like this a ripe for confusion

There's already a comment to dispel that confusion. If you want to nominate a replacement method name.....

[IamPete1]

couldn't you pass this class variable in from the function that calls this? That is to say remove this:

ardupilot/libraries/AC_Avoidance/AP_OADijkstra.cpp

Line 93 in bddb9aa

AP_OADijkstra_Error error_id;

And use the class variable in its place. I don't think you would need any of the other changes?

[rmackay9]

@IamPete1,

My solution would be to add a _last_error_id variable and pass that back in this one case. The slightly annoying part of my suggestion is that it means we'd need to set this _last_error_id everywhere we set err_id... so if you think you have a better idea then looking forward to it.

[peterbarker]

On Mon, 22 Jan 2024, Randy Mackay wrote: My preferred solution would be to add a _last_error_id variable and pass that back in this one case. The slightly annoying part of my suggestion is that it means we'd need to set this _last_error_id everywhere we set err_id... so if you think you have a better idea then looking forward to it.

This *was* by better idea - record the error code in a wrapper function :-)