Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add docking finished callback im MiRFM #33

Merged
merged 2 commits into from
May 31, 2023
Merged

Add docking finished callback im MiRFM #33

merged 2 commits into from
May 31, 2023

Conversation

xiyuoh
Copy link
Member

@xiyuoh xiyuoh commented May 31, 2023

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

Signed-off-by: Xi Yu Oh <xiyuoh@intrinsic.ai>
@xiyuoh xiyuoh requested a review from Yadunund May 31, 2023 07:55
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the other changes that were introduced in the other PR #31? And #29

Also looking at the code, there isn't any major difference between this CommandHandle and the one in fleet_adapter_mir. We should be able to reuse the same CommandHandle for both adapters after refactoring to pass a more generic transform object and the API class.
Having to update both files each time we fix a bug seems like something we can avoid. Or atleast reuse as much code as possible.

Signed-off-by: Xi Yu Oh <xiyuoh@intrinsic.ai>
@xiyuoh
Copy link
Member Author

xiyuoh commented May 31, 2023

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

👍 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
Copy link
Member

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
Copy link
Member Author

xiyuoh commented May 31, 2023

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
Copy link
Member

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.

@Yadunund Yadunund merged commit 0ba6205 into main May 31, 2023
@Yadunund Yadunund deleted the fix/mirfm_docking branch May 31, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants