search

open-rmf / fleet_adapter_mir

RMF/RoMi-H fleet adapter for the MiR API
Apache License 2.0
15 stars 15 forks source link

Add docking finished callback im MiRFM #33

Closed xiyuoh closed 1 year ago

xiyuoh commented 1 year ago

This PR adds a missing docking_finished_callback() when docking is aborted and returns response required for queue_dock_and_charge_mission.

xiyuoh commented 1 year ago

Ah ps missed those out. Other than those in edaf8f0, updating rmf_docking_executed according to api_response is already added in mirfm.

:+1: about reusing code. The main difference now is that the FM is retrieving individual robot information using the FM API, but the remaining APIs work pretty much the same. I guess in that case we can put them back in the same package then?

Yadunund commented 1 year ago

The main difference now is that the FM is retrieving individual robot information using the FM API, but the remaining APIs work pretty much the same. I guess in that case we can put them back in the same package then?

I'm not sure what the original reasoning was to separate the packages. Do you know?

To me we could have kept a single package and defined two ClientAPI classes, one for FM and the other for direct robot comms. Then the fleet_adapter_mir.py script can add an argparse variable to check if running with FM or not and accordingly instantiate the RobotCommandHandle with the right ClientAPI class. This way can reuse most of the code in the fleet_adapter_mir.py script too.

Anyways there is no problem with having two packages. We can have to separate API classes with the same functions (ideally both derive a base class) and we can reuse the same RobotCommandHandle for both adapters. Just need to pass the corresponding API class when instantiating the RobotCommandHandle. So in the FM adapter, instead of defining and importing a new RobotCommandHandle, we can just import fleet_adapter_mir.RobotCommandHandle. Similarly have fleet_adapter_mirfm.py import most of the common function definitions from fleet_adapter_mir.py.

xiyuoh commented 1 year ago

Initially it was just to make the MirFM adapter public asap as it was being requested. Now that you mention it, a base class sounds a lot better. Maybe we can keep the separate packages for the time being and i'll update it some time next week.

Yadunund commented 1 year ago

Initially it was just to make the MirFM adapter public asap as it was being requested. Now that you mention it, a base class sounds a lot better. Maybe we can keep the separate packages for the time being and i'll update it some time next week.

Yeah at the minimum let's open a ticket so that we remember to follow up.