-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Validate existence of custom_mode before setting #569
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
Conversation
dronekit/__init__.py
Outdated
| self.notify_attribute_listeners('armed', self.armed, cache=True) | ||
| if self._master.mode_mapping() != None: | ||
| self._flightmode = {v: k for k, v in self._master.mode_mapping().items()}[m.custom_mode] | ||
| if m.custom_mode in {v: k for k, v in self._master.mode_mapping().items()}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would prefer to avoid evaluating {v: k for k, v in self._master.mode_mapping().items()} twice
|
@liamstask I addressed your comment, could you review? |
|
@peterbarker Any comments? This looks good to me. |
|
couple of ideas come to mind:
pseudo-code: def _available_modes(self):
return self._master.mode_mapping().items()
def _is_mode_available(self, mode_name):
return True if mode_name in self._available_modes() else False
def _set_mode(self, mode):
self._flightmode = modewe can then change this to if _is_mode_available(m.custom_mode):
self._set_mode(m.custom_mode)this also helps cleanup the logic in some other parts of the Vehicle class, what do you guys think? @peterbarker, @tcr3dr, @sanderux ? |
|
@mrpollo I don't know if we do much checking of the modes internally, but from a public API point of view it would be useful to be able to test if a mode is available and to be able to iterate through the modes. |
|
dronekit-python should be the abstraction layer which allows you to run as many Python scripts on different autopilots as possible. Having the custom modes poke through to the API like this strikes me as a bad idea. Can we create our own list of types (presumably the union of ArduPilot and PX4 modes, renamed for consistency)? We would map from that type back to whatever the autopilot understands within DroneKit Python. Ramon's "mode_available" stuff would then apply to our own mode list rather than ArduPilot's or PX4's. If this is all too hard - I suggest we accept this patch to let people move forward. Ramon's suggestions would be a nice tidy. |
|
@peterbarker In practice I think that for DK there are really only two modes - AUTO and whatever is used most of the time for GUIDED (basically something which ignores stick input). My take on this is that we should allow all the modes from anything connected to push through, but that perhaps in our waiting "safe" methods we should set the recommended mode that is expected for the vehicle. That way the recommended approach will work for each vehicle, but they can still do anything else they like with modes. Just my two bits ... |
|
@hamishwillee @mrpollo @peterbarker Before a solid solution is created, would it be possible to merge this patch? I see no possibility for regression as the current approach simply fails when the custom_mode is unknown. The PX4/PX4-Autopilot#3790 PR is depending on this patch to be merged. Also i would like to try and get more of the PX4 community involved in getting PX4 / DK compatibility. Having this basic compatibility will help to get that started. /cc @LorenzMeier |
|
I support that. No compatibility break. We're no worse of if we accept this. Discussion can continue. My working week is over, but if this is still here on Monday with no strong objection I will merge this. @mrpollo Unless you get here first! |
|
@mrpollo Would be great to get this in today! |
|
I'm rebasing your work and pushing on master, see #577 |
|
hey @sanderux & @LorenzMeier this is now in master, do you guys need a new release? |
|
would probably help, although I'm not sure we "need" it to get unblocked - @sanderux ? |
|
@mrpollo Thanks for that. I will create a small tutorial in the http://dev.px4.io/pixhawk-companion-computer.html documentation. I will also put this on the agenda for our weekly dev call monday and try to get the community engaged. |
|
@mrpollo I reviewed basic functionality. Currently it seems to work mostly for mission handling and vehicle monitoring. Things like simple_goto and takeoff commands are not yet accepted by PX$, i need to dive further into the code to see if this is PX4 or DK related. I would appreciate a new release so that we can maximize involvement of the community that could help push the integration forward. |
This fixes exceptions when the autopilot identifies with an unknown custom_mode
This is required at least until PX4 custom modes have been added to pymavlink