Skip to content

Conversation

gwww
Copy link
Contributor

@gwww gwww commented Apr 25, 2020

Proposed change

This is a new integration for Universal Powerline Bus (UPB).

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

Additional information

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

@frenck
Copy link
Member

frenck commented Apr 25, 2020

Integrations with configuration flows, require a 100% coverage on at least the config flow. Please add tests 👍

@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

I messed up! In getting my next PR ready I pushed the wrong thing. Trying to back out to stable.

@bdraco
Copy link
Member

bdraco commented May 8, 2020

Thanks for not adding that to this PR. 👍 Quick note for your next PR that may save you some time: Firing custom events from entities is being phased out and won't be allowed in new code. https://developers.home-assistant.io/docs/device_automation_trigger/ may be a good option instead.

Co-authored-by: J. Nick Koston <nick@koston.org>
@bdraco
Copy link
Member

bdraco commented May 8, 2020

Looks good. I've give it another run though after I've had some sleep.

Please retest to make sure everything still works as expected.

@bdraco
Copy link
Member

bdraco commented May 8, 2020

@gwww Everything still working ?

@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

Nooooo! Having problems with the UPB_BRIGHTNESS_RATE_SCHEMA. I know this was working but is no longer working. I'm looking at it now.

@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

Somewhat minor problem. Trying to figure out how to make brightness and brightness_pct exclusive but require one of the two of them. I thought I copied the pattern from the light component but obviously missed something.

@bdraco
Copy link
Member

bdraco commented May 8, 2020

Somewhat minor problem. Trying to figure out how to make brightness and brightness_pct exclusive but require one of the two of them. I thought I copied the pattern from the light component but obviously missed something.

You shouldn't ever see brightness_pct as it should be converted to brightness in light/__init__.py:preprocess_turn_on_alternatives

    brightness_pct = params.pop(ATTR_BRIGHTNESS_PCT, None)
    if brightness_pct is not None:
        params[ATTR_BRIGHTNESS] = round(255 * brightness_pct / 100)

@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

It is the custom service schema that where the problem is, not the turn_on (which uses the ligth schema). Need to use has_at_least_one_key but trying to figure out how to use that with async_register_entity_service.

@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

I need some advice. Can't figure out if this is a core issue or if I need to do something different. I updated the schema to be:

UPB_BRIGHTNESS_RATE_SCHEMA = vol.All(
    {
        vol.Exclusive(ATTR_BRIGHTNESS, ATTR_BRIGHTNESS): VALID_BRIGHTNESS,
        vol.Exclusive(ATTR_BRIGHTNESS_PCT, ATTR_BRIGHTNESS): VALID_BRIGHTNESS_PCT,
        vol.Optional(ATTR_RATE, default=-1): VALID_RATE,
    },
    cv.has_at_least_one_key(
        ATTR_BRIGHTNESS, ATTR_BRIGHTNESS_PCT
    ),
)

When I call the service without brightness or brightness_pct, the exception I expect triggers but causes another exception:

2020-05-08 12:20:54 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection.4417002640] must contain at least one of brightness, brightness_pct.
Traceback (most recent call last):
  File "/Users/glenn/Development/automation/upb/upb-lib/.venv/lib/python3.7/site-packages/voluptuous/schema_builder.py", line 272, in __call__
    return self._compiled([], data)
  File "/Users/glenn/Development/automation/upb/upb-lib/.venv/lib/python3.7/site-packages/voluptuous/schema_builder.py", line 817, in validate_callable
    return schema(data)
  File "/Users/glenn/Development/automation/core/homeassistant/helpers/config_validation.py", line 115, in validate
    raise vol.Invalid("must contain at least one of {}.".format(", ".join(keys)))
voluptuous.error.Invalid: must contain at least one of brightness, brightness_pct.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/glenn/Development/automation/core/homeassistant/components/websocket_api/commands.py", line 130, in handle_call_service
    connection.context(msg),
  File "/Users/glenn/Development/automation/core/homeassistant/core.py", line 1207, in async_call
    processed_data = handler.schema(service_data)
  File "/Users/glenn/Development/automation/upb/upb-lib/.venv/lib/python3.7/site-packages/voluptuous/validators.py", line 208, in __call__
    return self._exec((Schema(val) for val in self.validators), v)
  File "/Users/glenn/Development/automation/upb/upb-lib/.venv/lib/python3.7/site-packages/voluptuous/validators.py", line 287, in _exec
    raise e if self.msg is None else AllInvalid(self.msg, path=path)
  File "/Users/glenn/Development/automation/upb/upb-lib/.venv/lib/python3.7/site-packages/voluptuous/validators.py", line 283, in _exec
    v = func(v)
  File "/Users/glenn/Development/automation/upb/upb-lib/.venv/lib/python3.7/site-packages/voluptuous/schema_builder.py", line 276, in __call__
    raise er.MultipleInvalid([e])
voluptuous.error.MultipleInvalid: must contain at least one of brightness, brightness_pct.

Functionally, the schema works correctly and catches the error I want to catch. The traceback in the hass console is what I don't know how to eliminate.

@bdraco
Copy link
Member

bdraco commented May 8, 2020

It looks like light.turn_on might be a better fit here. The standard is to use the existing core services when possible as other integrations like homekit, alexa, etc won't be aware of custom services. https://www.home-assistant.io/integrations/light/

light_fade_start -> light.turn_on
rate -> transition

light_fade_stop doesn't appear to be provided by light so that one is probably fine to leave as-is.

light_blink -> light.turn_on
flash

light_update_status -> homeassistant.update_entity. (You will need to rename async_light_update_status to async_update instead)
https://www.home-assistant.io/integrations/homeassistant/

@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

I haven't said so yet, but you have been super helpful! I really appreciate the help. Thank you.

I'm would like to but I am reluctant to remove light_fade_start as it is a specific message that can be sent to UPB devices. Different types of UPB devices treat the fade_start differently than turn_on. So that leaves the problem above that I am trying to figure out how to fix. One thing I could do is just remove brightness and only allow brightness_pct. This will also be a problem when I get to scene as it also has fade_start (and goto). To solve the double exception, I can't even thing where a try/except could be place as the exception is outside of my service code.

light_blink is different than the functionality of blink in turn_on in that the blink rate can be any value between 0 and 4.25 seconds.

update_entity -- Done! Will be in next commit when the brightness thing gets sorted.

@bdraco
Copy link
Member

bdraco commented May 8, 2020

I haven't said so yet, but you have been super helpful! I really appreciate the help. Thank you.

Glad I could help!

I'm would like to but I am reluctant to remove light_fade_start as it is a specific message that can be sent to UPB devices. Different types of UPB devices treat the fade_start differently than turn_on. So that leaves the problem above that I am trying to figure out how to fix. One thing I could do is just remove brightness and only allow brightness_pct. This will also be a problem when I get to scene as it also has fade_start (and goto). To solve the double exception, I can't even thing where a try/except could be place as the exception is outside of my service code.

https://github.com/alecthomas/voluptuous/blob/master/voluptuous/schema_builder.py#L276
I'm tempted to say the second exception is by design. There might be a way to avoid it, however I've yet to find an example that does.

light_blink is different than the functionality of blink in turn_on in that the blink rate can be any value between 0 and 4.25 seconds.

If the functionality is substantially different, it is fine to have a custom service.

update_entity -- Done! Will be in next commit when the brightness thing gets sorted.

👍

@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

Glad that I'm not the only person who gets the double exception. Should I call good enough? Nothing breaks. It validates the service call properly. The only bit it the double exception. Once this issue is clear I finish up a couple more tests and push a new commit.

@bdraco
Copy link
Member

bdraco commented May 8, 2020

Glad that I'm not the only person who gets the double exception. Should I call good enough? Nothing breaks. It validates the service call properly. The only bit it the double exception. Once this issue is clear I finish up a couple more tests and push a new commit.

That pattern is used in a few other places including rest/sensor.py so seems good enough. There is always the option to do another PR if a better solution is found.

Glenn Waters added 2 commits May 8, 2020 14:27
Update service schema to require one of brightness/brightness_pct.
Fix bug in setting brightness to zero.
Replace async_update_status and replace with async_update.
@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

I just submitted the latest changes. Appears that CI is down/broken/flaky.

@bdraco
Copy link
Member

bdraco commented May 8, 2020

/AzurePipelines Run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bdraco
Copy link
Member

bdraco commented May 8, 2020

@gwww Everything still working as expected?

@gwww
Copy link
Contributor Author

gwww commented May 8, 2020

All working!

@bdraco bdraco merged commit efb5296 into home-assistant:dev May 8, 2020
@lock lock bot locked and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants