osks / hass-gardena-smart-system

Moved to https://github.com/py-smart-gardena/hass-gardena-smart-system
Apache License 2.0
4 stars 2 forks source link

Full set of mower commands #10

Closed Jpsy closed 4 years ago

Jpsy commented 4 years ago

Oskar, first and foremost a big THANK YOU for your work!

Over the last days I have tested your component with my Gardena mower (which is my only Gardena Smart component). With my limited knowledge of Python and of the deeper mechanics of HA I have compiled the following details, which are hopefully correct:

The Husqvarna/Gardena API provides 4 mower commands:

  1. START_SECONDS_TO_OVERRIDE - Manual operation, use 'seconds' attribute to define duration.
  2. START_DONT_OVERRIDE - Automatic operation.
  3. PARK_UNTIL_NEXT_TASK - Cancel the current operation and return to charging station.
  4. PARK_UNTIL_FURTHER_NOTICE - Cancel the current operation, return to charging station, ignore schedule.

These 4 are all implemented as methods in your py-smart-gardena2 lib. But only 2 of them are exposed as HA services by your custom component:

  1. START_SECONDS_TO_OVERRIDE through services: vacuum.turn_on, and vacuum.start
  2. PARK_UNTIL_NEXT_TASK through services: vacuum.turn_off, vacuum.return_to_base, and vacuum.stop

My request is to expose all 4 commands.

The rationale is simply that they are needed. There is currently no service to stop the mower permanently (deactivate the schedule) and also no service that would activate the schedule without immediately starting the mower.

IMHO the best mapping would be this:

  1. START_SECONDS_TO_OVERRIDE = vacuum.start
  2. START_DONT_OVERRIDE = vacuum.turn_on
  3. PARK_UNTIL_NEXT_TASK = vacuum.return_to_base
  4. PARK_UNTIL_FURTHER_NOTICE = vacuum.stop and vacuum.turn_off

I tried implementing this myself to create a PR. But unfortunately I found that the turn_off and turn_on service calls never get through to the corresponding methods in vacuum.py of your component. I am far from capable of finding the reasons for that. Without the turn_on/off calls we would have only 3 services left to map 4 mower commands.

If in the end we would have to live with only 3 services, I would recommend the following mapping - which would provide the most important functions for automations:

  1. START_SECONDS_TO_OVERRIDE = vacuum.start
  2. PARK_UNTIL_NEXT_TASK = vacuum.return_to_base
  3. PARK_UNTIL_FURTHER_NOTICE = vacuum.stop

But it would be much more desirable to have all commands exposed. Please drop a comment if I can help in any way.

Jpsy commented 4 years ago

I just fount that in the HA dev docs chapter 3.18 vacuum class StateVacuumEntity the methods async_turn_on and async_turn_off are marked as "not supported". Is this the reason why they do not trigger the custom integration.? But they are called async* while the custom integration does not use the async prefix. My understanding is still much to thin.

wijnandtop commented 4 years ago

Maybe introduce a switch for the mower called "follow schedule".

The mapping of state will get a bit complicated though. For example I don't know if START_SECONDS_TO_OVERRIDE will return to schedule after the override seconds expire.

Jpsy commented 4 years ago

Hi @wijnandtop, such a switch would be great, but I think it is extremely difficult and error prone to implement. The main reason is that the schedule on/off state of the mower depends on several factors and there seems to be no mower command that can only change that state without any side effects. Also the commands are not context free.

For example: You would think that if the mower is parked and you call PARK_UNTIL_NEXT_TASK then you would reliably enable the schedule - without changing anything else. But in fact that command will change the schedule from today to tomorrow if the mower is already in scheduled state. So the stable way to enable a schedule when the mower is parked ist: PARK_UNTIL_FURTHER_NOTICE, wait for incoming status update, PARK_UNTIL_NEXT_TASK.

I am very much afraid we would invite evil with the trial to implement such a convenience function.

So my recommendation is to limit us to safely exposing the 4 existing commands if possible, or go with 3 commands if not. In fact your integration using the reverse engineer API also had only three commands exposed (through vacuum.start, .stop., .return_to_base).

What we should in fact do, is clearly document, which HA vacuum service calls which mower API command. So people will understand the difference between e.g. .start and .turn_on. I volunteer to write that documentation.

osks commented 4 years ago

IMHO the best mapping would be this:

1. `START_SECONDS_TO_OVERRIDE` = `vacuum.start`

2. `START_DONT_OVERRIDE` = `vacuum.turn_on`

3. `PARK_UNTIL_NEXT_TASK` = `vacuum.return_to_base`

4. `PARK_UNTIL_FURTHER_NOTICE` = `vacuum.stop` and `vacuum.turn_off`

That sounds like a good idea! I agree with you about how this should work.

I looked at the HA dev documentation you linked to and looked in the code for StateVacuumEntity in HA, and I found where it's says "Not supported" for async_turn_on, async_turn_off and async_toggle. I'm pretty sure that is only for the async version of those methods, and for for turn_on, turn_off, etc.

I haven't quite understood how a StateVacuumEntity is supposed to be controlled if it's not through the turn_on/turn_off methods (that you say didn't get through in your attempt). The dev documentation on the web (https://developers.home-assistant.io/docs/core/entity/vacuum) also doesn't say anything about the difference between VacuumEntity and StateVacuumEntity (both exist). I've also found the demo implementation for a vacuum (https://github.com/home-assistant/core/blob/dev/homeassistant/components/demo/vacuum.py), but haven't had time to think about it that much yet.

I found one difference though, the turn_on and turn_off methods in GardenaSmartMower doesn't have the **kwargs argument. Perhaps that is the problem?

Without knowing that you have already tested, I would try this:

class GardenaSmartMower(StateVacuumEntity):
    <... other code ...>

    def start(self):
        """Start cleaning or resume mowing."""
        duration = self.option_mower_duration * 60
        self._device.start_seconds_to_override(duration)

    def turn_on(self, **kwargs):
        """Start the mower."""
        self._device.start_dont_override()

    def return_to_base(self, **kwargs):
        """Set the lawn mower to return to the dock."""
        self._device.park_until_next_task()

    def stop(self, **kwargs):
        """Stop the lawn mower."""
        self.turn_off()

    def turn_off(self, **kwargs):
        """Stop mowing."""
        self.turn_off()
Jpsy commented 4 years ago

The changes that I proposed for a 3 command system are already running nicely in my local HA installation and with my mower. They are very similar to your code above.

I have also already tried to add the **kwargs param to turn_on and turn_off, but still no luck. They are not triggered by the corresponding service calls.

I am currently about to prepare a PR that will

Will need until tomorrow to get the PR up to your repo.

Jpsy commented 4 years ago

https://github.com/osks/hass-gardena-smart-system/pull/11#issue-424413435

Jpsy commented 4 years ago

This PR implements the 3 command version, but the turn_on and turn_off methods are still preserved for further testing. **kwargs has been added to both.

Jpsy commented 4 years ago

TODO: Check if there is a way to make vacuum.turn_on and vacuum.turn_off trigger the corresponding methods in vacuum.py. If yes: Implement 4 command version. If no: Stay with current version and close this issue.

Jpsy commented 4 years ago

Documentation for current 3 command version added in PR #19.

osks commented 4 years ago

Please continue in the new repository: py-smart-gardena/hass-gardena-smart-system#2