strands-project / strands_navigation

Higher-level navigation capabilities
41 stars 48 forks source link

Assuming closest node is current node #160

Closed hawesie closed 9 years ago

hawesie commented 9 years ago

Bob is not at any node, but wants to get to the ChargingNode. ChargingNode is the closest node (but not current). Top nav assumes that he's there already. Is that correct?

Jailander commented 9 years ago

No she is not assuming to be at the charging station she is not doing anything because there is no move_base action to the closest_node this is exactly what was happening on Friday when the robot was trying to do move_base to the charging_station fixing this would mean a very special case for navigation only and would force us to have all the nodes connecteted to the charging station with the same name, the best fix is to make the influence zone of the node previous to the charging station cover the place she might get stuck at.

hawesie commented 9 years ago

What about just calling move base to the closest node? Why wouldn't that work?

Jailander commented 9 years ago

Because the closest node is ChargingStation

Jailander commented 9 years ago

@hawesie isn't it? I can create a new set of actions for non precise movements that includes docking but is it urgent? how often would this happen?

hawesie commented 9 years ago

Yeah, good point, my brain is dead today. Maybe something like move base to the closest node with only move_base or person aware edges?

It's a bit of a show stopper because if this does occur then nothing can make the robot move.

Jailander commented 9 years ago

Ok, I think I can add a fix but would need testing

hawesie commented 9 years ago

It should be easy enough to recreate in simulation.

Jailander commented 9 years ago

@hawesie Yes but as I said somewhere I don't have a PC where I can test here, so I can test it but it would need to wait until the afternoon or I can fix it and someone else can recreate and test.

Also I was thinking of trying docking from there would it work can it see the robot station label?

hawesie commented 9 years ago

No it couldn't see the label. When we have a crash I may add some extra nodes around there.

Jailander commented 9 years ago

yes, sounds like the best solution, as navigating to the closest node with move base actions could be tricky since it would require modifying localisation to publish a sorted lists of nodes by distance and I can see potential navigation problem depending on the environment as the robot could be crossing non-existant edges :/ however I will push the previously suggested fix because I think it could work in other cases

hawesie commented 9 years ago

Looks like this killed our run again, even running with #162.

Jailander commented 9 years ago

Didn't it navigate to station? Is the output exactly the same???

Jailander commented 9 years ago

we saw the same behaviour tonight, but the reason was that cameras were dead (or very slow)

Jailander commented 9 years ago

I really don't see how it is posswould need to see some output meanwhile I'd recommend get the station node closer to the charging point

Jailander commented 9 years ago

Did you try making the influence area of the chargingPoint very small???

bfalacerda commented 9 years ago

no, i think that's the issue. What happens if he fails to undock, backtracks and finishes with current_node=ChargingPoint anyway?

Jailander commented 9 years ago

that is exactly why I asked if you made the influence area tiny?? also why would it fail to undock? only possible reason camera stuff, I have never seen it fail in that case

bfalacerda commented 9 years ago

fails to dock i mean. after he fails to dock, he backtracks and docking reports failure. What happens if he is still in ChargingPoint? - our influence area is still pretty big...

Jailander commented 9 years ago

Yes I made it tiny, and the station point closer, we'll find a solution afterwards??

hawesie commented 9 years ago

We'll make ours tiny tomorrow too. We meant to do it before the run today but forgot.

Jailander commented 9 years ago

yes that made the trick in Linda, pity we never saw this happening before :( it's been like this since before the review meeting

cburbridge commented 9 years ago

This was a pretty big bug during the marathon, I remember discussing it with @bfalacerda a lot at the time and thinking it required a "propper" fix of not just considering location as the only indicator of which node you are in, but also considering other aspects such as charging status. If this is still in the system we should focus more on fixing this bug....

Jailander commented 9 years ago

Yes we also talked about it but that would mean that we are making the navigation scitos dependant again and @bfalacerda made a big effort to make the monitored navigation independent of the platform, I think we can discuss having special locations that listen to specific topics (i,e charging to say you are at the charging station), but this special case would still be tricky because the docking action cannot be triggered from anywhere.

On the other hand the fix that we made during the marathon fixes the issue for now. And I think that the feature proposed #185 can solve it definitely by using recovery behaviour that can plan its way back to a position it can trigger the docking from.

marc-hanheide commented 9 years ago

If I may chip in my view: In general, this is talking about topological localisation (which is separate form navigation, as right now implemented in localisation.py. So, in general localisation is a problem of collecting sensor-based evidence form the environment to know which node your in. So, conceptually, a metric pose or an electric pulse both qualify ;-)

In the concrete case: I think the point_in_poly check is only one of many different functions to check where you are. IMPO topo-loc should be able to support different such methods. Maybe it's the default, but we could easily have other check functions to be tight to nodes (i.e. be configured per node). This could even be a list of check functions that all need to be true to say you are at a place. Practically, this doesn't have to introduce scitos_apps as dependency as one could just parametrise a test by a topic and a simply "selection filter".

BTW: I find this line

#Charging Point exception it should never be closest node

very suspicious and preventing our system to be used by other people. This could be resolved by not allowing it to use the point_in_poly test, but rather my proposed check_topic_content as configured above.

Jailander commented 9 years ago

That wouldn't solve the problem, as the problem was that when the robot's closest node (not current) was the charging station it did move_base to it, as I said that line was the temporary solution and its still required in topological navigation, in policy_execution we consider the case of the robot navigating to the node that is closest with move base actions and in issue #185 we wanted to have specific recovery behaviours for each edge and we could make sure that the robot always goes back to a safe point with specific recovery actions, that should solve the problem. However I see no problem implementing what @marc-hanheide suggest and I think it would be very useful, but we would need to see how to define this kind of things in the topological map, (I don't think it should be a parameter it has to be something intrinsic to the node itself).

marc-hanheide commented 9 years ago

OK, I see. Sorry for getting off-topic.

Jailander commented 9 years ago

No, don't be!! I really like that idea, because it might be useful for different types of nodes. I think I should look into now that we are working on #169 maybe I would need some feedback form @bfalacerda for this too.

bfalacerda commented 9 years ago

I think we could have an extra field in the topological node that is basically a topic that needs to be publishing true for the waypoint to be closest or current. Then, for the charging point we could add the /charging topic (or whatever it's called). With this we keep top nav scitos independent, and also have some flexibility to add different conditions for other waypoints that might need them

Jailander commented 9 years ago

I think we can have two extra fields on each node one for 'special cases' like charging and these things and one for localising by topic like @marc-hanheide {topic: '/battery_state', attr_path='data.charging', eq_val='True'} in such case it would be very easy to generalise, what do you think?

bfalacerda commented 9 years ago

Why do we need two different cases?

Jailander commented 9 years ago

well I was thinking that 'special cases' are caused by the needed actions to reach the node (e.g. docking for charging) whilst the localisation by topic is caused by the specifications of the node itself. But as I am writing this I can't think on any case on which these nodes wouldn't be 'special'. So the answer is you are right :P

hawesie commented 9 years ago

@Jailander wrote in #208 :

This issue is a duplicate of #160 is going to be solved by using (https://github.com/strands-project/strands_navigation/blob/indigo-devel/strands_navigation_msgs/msg/TopologicalNode.msg#L9)

When you say "going to be", when is this going to happen?

Jailander commented 9 years ago

probably today, can't promise, it will also mean running the migrate script again

Jailander commented 9 years ago

216 should fix this

bfalacerda commented 9 years ago

fixed by #216