Suggest we decouple direct colision feature from move_basic

[mjstn]

In general it may be time to have move_basic only use sonar data for modifications to path planning and not try to outsmart the 'danger of collision' function required on most systems. The text below was a post to our slack channel but am keeping it here in case this issue becomes prioritizd as an actionable issue.

it is time to re-think and remove all the nasty sonar baggage from move basic as we form a 'move basic' that is a more sharable component. The thought: Have a node that listens to Sonars and limit switches and makes high level decisions and perhaps even very basic costmap data that it broadcasts on some topic. Move basic or whatever it becomes such as 'move_smart' perhaps gets decoupled from emergency stop type activities. It may still need sonars for route plan changes but just don't use sonars in move xxxx for full stop stuff. Here is an interesting approach to consider. I have felt that we use move_basic for a semi-smart driving system that always accepts commands from 'above' where above is aisle mode, EZmap or breadcrumb (so far). We should NOT have sonar logic to stop move basic be within move basic, it makes it not at all portable (for code and higher level apps that use it). What to do about collision is a higher level app logic. THEREFORE: I suggest we as a group consider that we place any logic that makes decisions about combining sonar data into logic that will need to abort move base as a separate node. The collision node deals with looking at sonars and at bumper switches and perhaps other proximity sensors and decides when it is time to 'alert' higher level code through a topic to be determined. At this time I made sonar_detector as you are aware. I am not suggesting that, I am suggesting a decouple of the nasty collision stuff sort of like the purpose of sonar_detector. See it's readme. Perhaps we need some node like that that deals with sonars and makes decisions suitable for our main apps like EZmap, Breadcrumb and Polaris. We put the function of that node to notifiy whatever higher level app that 'we gonna hit sumpin boss'. Then we make the higher level boss tell move_basic to shutup. It is also possible to have this node be listened to by move_basic itself for extra safety. We could choose to have a well known topic such as sonar_detector invented that is called system_control for the sonar_detector and now ubiquity motor. Anybody can listen for basic 'Stop NOW or you will be sorry' messages. sonar_detector is raw basic stuff so I am suggesting we thing out a spec for something that fills that need but fits our 3 top apps. This is the sort of thing that will also de-clutter move basic and allow us to make move basic more general for our different apps. WHAT SAY YEE??? 12:24 If we get a quarum then what I have just said and your other requirements go into a new issue under move_basic perhaps. This will be lost in a month or so so I just post here to see if other feel this is ok or not. PS: I realize it is now Saturday but what @vid said caused other thoughts I have been having on sonars in move_basic to sort of 'gel into a requirements definition' of sorts. (edited)

[dorkamotorka]

A copy of my comment on Slack, to not get lost:

If we "move" the movement part from move_basic to motor_node I think that's good since we will have a dynamic model and a controller in motor_node defined(programmed) anyway(Gen 6). What we should also consider is that its should be general enough (expose so many parameters), that it will be easily configurable also for Breadcrumb and any other Robot creation we come up with in future. Otherwise it will be to much Magni specific and not any other Robot type will ever be able to work good with the algorithm. Also I wouldn't modify move_basic to a point where it is not performing anymore driving maneuvers(just collision detection a.k.a sonar node) since it has served a great purpose for our projects. I would suggest to create a collision detection node and implement the control algorithm in motor node(which will be totally different with what is currently done in move_basic) as David proposed, but without modifying the move_basic at all.

[JanezCim]

Things that might me usefull to add into consideration a) showing on toucscreen and/or webapp that you have safety stopped and why b) think about how this is going to interact with any "mode manager" now and in the future (think aislemode, random walk, teleop mode, ...) c) when you safery stop, what moves do you allow? Backward only? Turning? How does move_basic know that? d)...

This usually becomes quite a complicated architectural feat when you think about the whole system so it would require input from multiple people.

[dorkamotorka]

Its not actually an architecture I made but I think that in this case we can benefit from twist_mux (multiplexer) majorly.

I think this as easy and general we can go. This enables us also to use collision avoidance when we are controlling the robot with joystick/keyboard it enables multiple sources(Remote STOP, STOP button on robot, collision avoidance node) to block the /XXX_vel to transfer to the motor_node.

I think this would also imply least changes to the current move_basic.

[rohbotics]

@tp4348 That is a pretty standard architecture in ROS, and we should definitely implement it. http://wiki.ros.org/twist_mux already exists, and allows for different inputs to have different priorities.

As for getting the collision stuff out of move_basic, I agree that we should have a new node that does the basic level of this. Something that takes in the requested velocity, evaluates the safety and sets a lock out if necessary. But move_basic needs significantly more information than that, because it uses obstacle information to slow down early, determine if turns are possible, etc. So if we really want to completely decouple the obstacle detection, we will need to find a way to publish everything move_basic wants. This will likely be either in the form of an occupancy grid (costmap), or in a obstacle list (a list of points and lines that represent detected objects). The obstacle list is what move_basic uses internally today.

[JanezCim]

I agree with @rohbotics , twist mux in its as-is form does not allow turning while safety stop is enabled, while move_basic does, which is a super usefull feature and would be bad to loose it.