b-it-bots / mas_perception

2 stars 13 forks source link

refactor mcr_door_status into mdr_enter_door_action #1

Closed minhnh closed 6 years ago

minhnh commented 6 years ago

Since mcr_door_status only subscribes to the front laser topic to check on the front distance if it's larger than a threshold, it really doesn't warrant being in a separate (C++ no less) package. We can make it a state of the enter door action until we need to reuse it elsewhere (unlikely).

This could be a simple task for the new guys.

@alex-mitrevski @argenos

argenos commented 6 years ago

I think that has some assumptions about our skill-based architecture, however mcr_door_status is potentially being used somewhere else where the action server does not make sense. I would leave it as is for now. Besides that, there are more pressing concerns in my opinion.

minhnh commented 6 years ago

Well I grepped the whole src folder in my workspace and the only mention is in the enter door action. The code is also really just a subscriber to the laser scan and a publisher to door status, which is a lot of overhead. We can have a state in the enter door action which subscribes to the laser scan.

On the second point about there are more pressing concerns, that's why I think it's a good exercise for the newcomers to work on, since I'm more interested with the perception_msgs and scene_segmentation packages.

argenos commented 6 years ago

We are definitely using this in one of the projects, plus it's also up to @b-it-bots/work . In my opinion, we should not embed this into an action

argenos commented 6 years ago

One other point... I don't think everything is an action, we might want to drive the robot around and just note that the door is open/closed without wanting to enter it. This might be relevant information for the world model, for example.