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 OZW node config parameters websocket commands #40527

Merged
merged 39 commits into from Oct 5, 2020

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Sep 24, 2020

Proposed change

I added two new websocket commands that can retrieve and set config parameters for a given node, with the idea being that someone with frontend experience could extend the OZW config panel to include a more friendly UI for displaying the available parameters and updating them as needed.

TODO:

  • Add tests
  • Run local tests to verify this works

Documentation PR: home-assistant/home-assistant.io#14793

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@project-bot project-bot bot added this to Needs review in Dev Sep 24, 2020
@raman325 raman325 marked this pull request as draft September 24, 2020 07:54
@raman325 raman325 changed the title Add OZW websocket commands to retrieve and set config parameters for a node WIP: Add OZW websocket commands to retrieve and set config parameters for a node Sep 24, 2020
@MartinHjelmare
Copy link
Member

Thanks for the PR!

There needs to be a plan for the frontend too before we'll add the supporting backend APIs. A config panel is on our roadmap and @cgarwood has already added an MVP implementation of that. @cgarwood have we already planned out this feature?

@raman325
Copy link
Contributor Author

Fair, and to be honest I have not talked to cgarwood about this, I assumed it was intended to be added in the future and wanted to give it a headstart so I could learn about the integration. I will pause on this from now until he responds.

BTW, I also planned to add API endpoints to set, clear, and get usercodes for locks, but I will hold off until I hear back

@cgarwood
Copy link
Member

This was next on my list for the config panel but I haven't started on it yet, so having the backend in place already helps a ton 😃

@raman325
Copy link
Contributor Author

Great! I was thinking some more about this and thought maybe we should pull the logic into a single method that then both the service and WS API could call. That should reduce the maintenance required over the backend for this.

You ok with me submitting similar changes for usercodes in a separate PR then? I can start working on that after I finish cleaning this one up.

@cgarwood
Copy link
Member

Yeah, that would be great. We could probably do the same combined logic with the set_usercode function as it makes sense to have it available as a service for automations too.

@raman325
Copy link
Contributor Author

@cgarwood how does this look? I added some additional error checking along the way and incorporated #40433 in as well

@MartinHjelmare MartinHjelmare changed the title WIP: Add OZW websocket commands to retrieve and set config parameters for a node WIP: Add OZW node config parameters websocket commands Sep 24, 2020
@raman325 raman325 marked this pull request as ready for review September 24, 2020 17:47
@raman325 raman325 changed the title WIP: Add OZW node config parameters websocket commands Add OZW node config parameters websocket commands Sep 24, 2020
Dev automation moved this from Needs review to Reviewer approved Oct 2, 2020
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare
Copy link
Member

@cgarwood can we merge here?

@raman325
Copy link
Contributor Author

raman325 commented Oct 2, 2020

Actually looks like I made a mistake with the BitSet implementation as it expects a bool, not 0 or 1: https://github.com/OpenZWave/qt-openzwave/blob/master/docs/MQTT.md#setvalue. I will pull that out of this PR and submit a fix for openzwavemqtt

@kpine
Copy link
Contributor

kpine commented Oct 2, 2020

Actually looks like I made a mistake with the BitSet implementation as it expects a bool, not 0 or 1: https://github.com/OpenZWave/qt-openzwave/blob/master/docs/MQTT.md#setvalue. I will pull that out of this PR and submit a fix for openzwavemqtt

The BitSet value is also a list/array of dicts. The _set_bitset_config_parameter it's not implemented that way is it? It looks like it only accepts a single bit (dict). The get seems to return a list.

@raman325
Copy link
Contributor Author

raman325 commented Oct 2, 2020

Actually looks like I made a mistake with the BitSet implementation as it expects a bool, not 0 or 1: https://github.com/OpenZWave/qt-openzwave/blob/master/docs/MQTT.md#setvalue. I will pull that out of this PR and submit a fix for openzwavemqtt

The BitSet value is also a list/array of dicts. The _set_bitset_config_parameter it's not implemented that way is it? It looks like it only accepts a single bit (dict). The get seems to return a list.

The way I had implemented it, it will support multiple bits as a dict:
<bit #1 position>: <true/false>
<bit #2 label>: <true/false>

I can switch this to a list of dicts instead if we want to it to match what qt-openzwave expects, I just thought this implementation was more convenient to use in cases where a user wants to call the service

@raman325
Copy link
Contributor Author

raman325 commented Oct 2, 2020

Actually I will go ahead and change it to a list of dict's because it makes openzwavemqtt's job easier

@kpine
Copy link
Contributor

kpine commented Oct 2, 2020

OK. I don't quite understand, but the MQTT value sent to ozwdaemon must be a list/array of dicts, it can't be a single dict.

@raman325
Copy link
Contributor Author

raman325 commented Oct 2, 2020

Yeah the bitset implementation needs more work

@raman325
Copy link
Contributor Author

raman325 commented Oct 2, 2020

Yeah the bitset implementation needs more work

this has been resolved. I can either wait until the upstream PR (cgarwood/python-openzwave-mqtt#95) gets merged and a new patch version gets released to reintroduce the changes to support BitSet here, or I can submit a separate PR. The only change is adding the bitset value input to the WS API and service call schemas

@raman325
Copy link
Contributor Author

raman325 commented Oct 3, 2020

Went ahead and re-introduced bitset support since the upstream PR was merged and the version was bumped

@raman325 raman325 mentioned this pull request Oct 4, 2020
21 tasks
Dev automation moved this from Reviewer approved to Review in progress Oct 5, 2020
Dev automation moved this from Review in progress to Reviewer approved Oct 5, 2020
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare MartinHjelmare merged commit 6db4075 into home-assistant:dev Oct 5, 2020
Dev automation moved this from Reviewer approved to Done Oct 5, 2020
@raman325 raman325 deleted the ozw_ws_config_parameters branch October 5, 2020 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants