Alleged sonar avoidance fixes

[MoffKalast]

So in order to make the robot stop at obstacles that the lidar doesn't see I've increased the min stopping distance for the obstacle detector to 25cm (since the min for sonars is 23cm and even that's debatable) in the forward/back direction and to 15cm sideways. I've also increased the max time that would disqualify an old sonar message, which seemed to help.

In general I can't really test it properly in gazebo since the collision implementation simply fails when an obstacle gets too close and the ray test goes through the mesh, detecting the faces on the other side (of a box for example). #justgazebothings

The questions is more how does this affect aisle mode, although from my tests in Gazebo it doesn't seem to really react any differently at all. Nor does it actually stop at such obstacles...

Anyhow it would need to be tested on @mjstn's polaris to check what actually happens.

Also fixed some indents, that's why the changelog appears weird.

[mjstn]

We have a bit of a platform unique issue here. The shipping Magni product has sonars that can see things 6cm away. It is only the 'waterproof' sonars of Polaris that have that extremely poor trait of not being able to judge distances closer than the 23cm ish number.

[JanezCim]

yeah so here are the parameters that are currently set from aisle_mode.launch for move basic:

<node name="move_basic" pkg="move_basic" type="move_basic"  output="screen">
    <param name="base_frame" value="base_link"/>
    <param name="angular_tolerance" value="0.2"/>
    <param name="linear_tolerance" value="0.15"/>
    <param name="max_linear_velocity" value="0.2"/>
    <param name="velocity_threshold" value="0.1"/>   
    <param name="max_turning_velocity" value="0.4"/> <!--max angular speed while robot is turning on the spot-->
    <param name="min_turning_velocity" value="0.2"/> <!--min angular speed while robot is turning on the spot-->
    <param name="max_lateral_velocity" value="0.4"/> <!--max angular speed while driving towards goal-->
    <param name="angular_acceleration" value="0.05"/>
    <param name="linear_acceleration" value="1.0"/>
    <param name="reverse_without_turning_threshold" value="0.01"/>
    <param name="lateral_kp" value="1"/>aisle_cmd_vel_topic

    <param name="forward_obstacle_threshold" value="0.25"/>
    <param name="robot_back_length" value="0.4"/>
    <param name="min_side_dist" value="$(arg robot_radius)"/>
    <param name="robot_width" value="$(arg robot_radius)"/>
    <param name="robot_front_length" value="0.25"/>
    <remap from="cmd_vel" to="$(arg aisle_cmd_vel_topic)"/>

  </node>

TLDR: You didnt change anything that was set from that launch. Lets try this on 4WD robot and see what happens.

Long Clarification: So the thing is that the safety stop, afaik is set with forward_obstacle_threshold and min_side_dist params, and I didnt actually bother to set the params no_obstacle_dist in the collision collision_checker.cpp. This is because I didnt dig up what these parameters are supposed to mean since there was so many changes in the nav-features branch. With @tp4348 we wanted to clear this up in https://github.com/UbiquityRobotics/move_basic/issues/77 but came to the conclusion that this would anyway be obsolete in the newer versions of move_basic somewhere. So imho for this branch this would have to be separately debugged with a physical robot.

[MoffKalast]

@JanezCim Ah I see I've gotten slightly ahead of myself. Probably best to disregard this branch in that case and merely set the existing params in the launch.

It does seem interesting that by default the forward_obstacle_threshold is set to half a meter, perhaps we should use that instead? Does it block aislemode functionality too much in that case? If anything we should be increasing the distance with these sensors, not decreasing it.

[JanezCim]

forward_obstacle_threshold, robot_width, forward_obstacle_threshold really influence aislemode and are set precisely so the robot does not get stuck in tight corridors. So if you are changing those, a lot of extra testing will be needed.

With @tp4348 we were briefly discussing to set the aislemode on the branch nav-features which seems to be much better now, maybe you can make a branch off that and we try with that one?

[MoffKalast]

Well if you're going to be switching branches then a lot more testing will be needed anyway. And the current precise setting kind of makes the robot hit stuff in obvious huge-box-in-front-of-robot cases so it's gonna need to be patched in some way.

But if I understood correctly what you wrote before then this branch is unnecessary and all modifications can be done on the avalon repo.

[JanezCim]

But if I understood correctly what you wrote before then this branch is unnecessary and all modifications can be done on the avalon repo.

[dorkamotorka]

It would be great if we can use branch nav-features for all of our project, since this will eventually become the main branch I guess and this way the project will end up using main branch. Probably easier than said, but if there are navigation stuff that we are using successfully for goal-routes but not for avalon, I want to be aware of it. In my opinion it should just be the case of launch parameters.

[mjstn]

At a high level we are going to always struggle from now on with features or parameter changes required for one branch and in the worse case differences in how things are logically done between products (such as URDF and 4WD driven things). For things as simple as detection distance try really hard to do that in launch files and ROS params because, hey, that is what they are there for. For things like how move_basic operates from the code standpoint is where it can get 'ugly'.

So to not try to solve the world's problems for this issue, lets agree that difference in detection of obstical is ok to deal with in launch file params.

After all is said and done the OTHER REAL work (besides fixing one thin on one product) that is often overlooked is that we make a change to a common thing like move_basic or ubiquity_motor and an 'honest' gentlemans agreement would be we must regression test in ALL products used for changes that are not extremely trivial. Even trivial changes need at least a simple unit test in the other products that exercises the simple logic change applicable to the 'simple' change (if there IS such a thing).

In short we have no way to track multi-product regression tests NOR do we have a test 'group' that can do all that stuff.

This I would like to think we strive to make better in 2021 and is simply a part of 'company growing pains'.

[JanezCim]

@MoffKalast whats the plan with this then?

[MoffKalast]

Don't ask me, I closed this PR, you guys reopened it for some reason,

[JanezCim]

@tp4348 ?