AP_Terrain: Protect against division by 0 when TERRAIN_SPACING is less then 1

[tiagoc-santos]

This fixes #29392. In the calculate_grid_info function and the east_blocks function of AP_Terrain, the TERRAIN_SPACING parameter is used to determine grid spacing and how many blocks are needed in a stride for a given location.

When this parameter is set to a value less than 1, a division by zero occurs, leading to an arithmetic exception. This fix ensures that grid_spacing is properly handled when its value is below 1. Specifically, if grid_spacing is less than 1, the system defaults to using a value of 1 instead.

This fix has been tested in SITL environment and seems to be working fine.

[peterbarker]

This probably needs to move to being a float at some stage to allow for sub-metre terrain grids.

When we do that we'd have to find all of these places again, which would be unfortunate.

Did you consider throwing a config_error() instead of constraining the value like you've done here? ie. force the user to choose a valid value....

[tiagoc-santos]

This probably needs to move to being a float at some stage to allow for sub-metre terrain grids.

When we do that we'd have to find all of these places again, which would be unfortunate.

Did you consider throwing a config_error() instead of constraining the value like you've done here? ie. force the user to choose a valid value....

I hadn't considered it, but I do agree, throwing a config_error() seems like a cleaner approach for this.

I can give it a try. Where do you think would be the best place to throw it?

[tiagoc-santos]

Hi @peterbarker.

I've been digging into the suggestion you made and I have a few questions.

Should I do the verification (and subsequently throw the config_error() ) inside the "problematic" functions? If so, do they need to return? Or is there a better place to make this check?

Thanks!

[peterbarker]

Should I do the verification (and subsequently throw the config_error() ) inside the "problematic" functions?

Hmm. Doesn't look like terrain has a .init() function in it. That's where it should go.

So I think not with config_error.

Since this should be short-term until we change the spacing to be float, we could probably add a pre-arm check for it. So that's just an additional clause in AP_Terrain::pre_arm_check

It's entirely possible when we take this solution to DevCall that the response will be, "no, just make it a float".

Did you want me to take this to DevCall before you do the work instead of after?

[tiagoc-santos]

Hmm. Doesn't look like terrain has a .init() function in it. That's where it should go.

So I think not with config_error.

Since this should be short-term until we change the spacing to be float, we could probably add a pre-arm check for it. So that's just an additional clause in AP_Terrain::pre_arm_check

It's entirely possible when we take this solution to DevCall that the response will be, "no, just make it a float".

Did you want me to take this to DevCall before you do the work instead of after?

I don’t mind doing the pre-arm check, but I agree it might be worth bringing it to DevCall first to see what the consensus is. If you’re okay with that, feel free to raise it there before I dive into the patch.

[tridge]

@tiagoc-santos please change it so that AP_Terrain::pre_arm_checks() return false with an error message if spacing <= 0

[tiagoc-santos]

@tiagoc-santos please change it so that AP_Terrain::pre_arm_checks() return false with an error message if spacing <= 0

Done! Let me know if you need me to rebase and/or squash the commits.

[peterbarker]

@tiagoc-santos please change it so that AP_Terrain::pre_arm_checks() return false with an error message if spacing <= 0

Yep, that's the ticket.

Done! Let me know if you need me to rebase and/or squash the commits.

I've just done that (before reading your offer here - sorry!)

Marking as MergeOnCIPass