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

notify.homematic #16973

Merged
merged 10 commits into from Oct 12, 2018

Conversation

Projects
None yet
5 participants
@siom79
Contributor

siom79 commented Sep 29, 2018

Description:

This pull request adds the homematic signal generator as notify component.

Related issue (if applicable): n/a

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#6379

Example entry for configuration.yaml (if applicable):

notify:
  - name: my_hm
    platform: homematic
    address: NEQXXXXXXX
    channel: 2
    param: "SUBMIT"
    value: "1,1,108000,8"

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Show resolved Hide resolved tests/components/notify/test_homematic_signalgen.py Outdated
ATTR_PARAM: "SUBMIT",
ATTR_VALUE: valueForSigGen
}
_LOGGER.debug('Calling service: domain=' + DOMAIN + ';service=' + SERVICE_SET_DEVICE_VALUE +

This comment has been minimized.

@houndci-bot

houndci-bot Sep 29, 2018

line too long (100 > 79 characters)

valueForSigGen = template.render_complex(templ, None)
if valueForSigGen is None:
_LOGGER.error("Value is null: Not invoking homematic signal generator.")

This comment has been minimized.

@houndci-bot

houndci-bot Sep 29, 2018

line too long (84 > 79 characters)

@siom79 siom79 referenced this pull request Sep 29, 2018

Merged

Homeatic signalgen #6379

2 of 2 tasks complete
@pvizeli

Call this platform homematic and use the discovery feature to auto setup it.

@siom79

This comment has been minimized.

Contributor

siom79 commented Sep 30, 2018

@pvizeli: I need to configure which MP3 track on the device to play in case of a notification. How does discovery work in this case? How do you mean that?

@pvizeli pvizeli self-assigned this Oct 1, 2018

@siom79

This comment has been minimized.

Contributor

siom79 commented Oct 1, 2018

@pvizeli: Just to explain my intentions: This pull request aims to provide support for Homematic's signal generator "HM-OU-CFM-TW" as notification platform. The signal generator is already supported by the pyhomematic library: https://github.com/danielperna84/pyhomematic/blob/master/pyhomematic/devicetypes/actors.py#L461 and is discovered automatically. But like for the Kodi notification component, I would like to play a mp3 file stored on the Homematic signal generator (or let the LEDs blink) for example in case the washing machine is ready or the front door opens when the alarm mode is enabled. For Kodi there are also two "components": One for notifications and one to control the device itself. The implementation to control the signal generator is already available. This one provides support for notifications by invoking the already existing homematic service. Hence, it is not a complete new platform.

Beyond that I decided to name the notification homematic_signalgen because it only addresses the signal generator. This eases its ussage as you can use it without having to know the channel and the datapoint param. Hence, you can have in the future more than one "notification component" for homematic. On the other hand, it would be also possible to implement a "generic" homematic notification, but in that case you would have to know how to address the signal generator and you could also send notifications to your covers or switches, which would not make much sense.

Let me know what you think.

@danielperna84

This comment has been minimized.

Contributor

danielperna84 commented Oct 1, 2018

Isn't it possible to play MP3s by using the homematic.set_device_value service? It's intended for those cases where a dedicated platform for just a single device would be needed. There are other HomeMatic devices which in theory would deserve their own platform as well. But as long as using the mentioned service as a workaround does the job, I don't think dedicated platforms are necessary.

Apart from that: I have updated pyhomematic to 0.1.50, so that would need to be changed in the requirements. And I wonder how tests pass. Your tests don't initialize the virtual CCU. So setting up the component in the tests should actually fail. 🤔

@siom79

This comment has been minimized.

Contributor

siom79 commented Oct 2, 2018

@pvizeli

This comment has been minimized.

Member

pvizeli commented Oct 2, 2018

Kodi is bad. You can use group notify to set fix value and make notify.homeassistant variable over data. That is why group notify platform was burn.

@siom79 siom79 force-pushed the siom79:homematic-signalgen branch from a3c1c34 to 2f3c46f Oct 3, 2018

@siom79 siom79 changed the title from Homematic signalgen to notify.homematic Oct 3, 2018

@siom79

This comment has been minimized.

Contributor

siom79 commented Oct 3, 2018

@pvizeli OK. I have renamed the notify component to notify.homematic and made it generic. You can now pass also channel and param:

notify:
  - name: my_hm
    platform: homematic
    address: NEQXXXXXXX
    channel: 2
    param: "SUBMIT"
    value: "1,1,108000,8"

Let me know what you think.

The group notification only allows to call other notifications from the notify domain: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/notify/group.py#L68. Maybe it makes sense to allow calling "normal" services, too?

@danielperna84

This comment has been minimized.

Contributor

danielperna84 commented Oct 3, 2018

The code still has references to the Signal generator, which isn't correct anymore now that you can specify the param and possibly can control all kinds of devices. So that's a cosmetic improvement that should be done

@siom79

This comment has been minimized.

Contributor

siom79 commented Oct 3, 2018

@danielperna84 OK. I have removed the terms signal generator from the sources. 😉

@pvizeli

This comment has been minimized.

Member

pvizeli commented Oct 5, 2018

@siom79 you can use data to pass additional parameters and targets for device target.

With groups, you can create a simple notify.homematic_mydoor and pass all data to this group and after that you can call this with a simple service call to this service. It allow also to have a service per mp3 like you want have but it is a very flexible solution and every body can build his own stuff around this.

@danielperna84

This comment has been minimized.

Contributor

danielperna84 commented Oct 5, 2018

@pvizeli
This currently does not work though because the service has to be a notify service:

Taken from here:

service (Required): The service part of an entity ID, i.e. if you use notify.html5 normally, just put html5.

I haven't tried it myself, but the statement of the documentation is, that you can't use any other service types besides notification-services. Maybe internally service: homematic.set_device_value would work. But at first sight it does not. Hence this notify platform would still make sense.

Although I would rather see this as a single generic service (just notify.homematic and nothing else) that automatically gets created, and with every call also the device address would need to be provided. So essentially the same as homematic.set_device_value, but within the notify-domain (which apparently allows to do stuff a regular service can't to).

@siom79

This comment has been minimized.

Contributor

siom79 commented Oct 5, 2018

@pvizeli: The final goal is to use an alert together with a notification like in the following example:

alert:
  washing_machine:
    name: Washing machine ready
    done_message: Washing machine done
    entity_id: binary_sensor.washing_machine_ready
    state: 'on'
    repeat: 10
    can_acknowledge: True
    notifiers:
      - my_hm_signalgen
      
notify:
  - platform: homematic
    name: my_hm_signalgen
    address: NEQXXXXXXX
    channel: 2
    param: "SUBMIT"
    value: "1,1,108000,8"

Please note that the example above already uses this pull request and does not work without it.
But you say that I can call any service using the group notification. But how?
The following example causes an "Unable to find service notify/homematic" error:

alert:
  washing_machine:
    name: Washing machine ready
    done_message: Washing machine done
    entity_id: binary_sensor.washing_machine_ready
    state: 'on'
    repeat: 10
    can_acknowledge: True
    notifiers:
      - my_hm
      
notify:
  - platform: group
    name: my_hm
    services:
      - service: homematic
        data:
          data:
            address: NEQ12341234
            channel: 2
            param: SUBMIT
            value: "1,1,108000,3"

It's because you can invoke from the alert component and the group notification component only services in the notify domain.

@pvizeli

Perfect. If you want it, I'm also fine if you want it make again a bit static with fixed value in a future PR, if you see that the handling is not usefully 👍

@pvizeli pvizeli merged commit cb3d62e into home-assistant:dev Oct 12, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.04%) to 93.664%
Details

@wafflebot wafflebot bot removed the in progress label Oct 12, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment