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 vocolinc flowerbud chars #23

Merged
merged 6 commits into from
Oct 24, 2020
Merged

Conversation

adrum
Copy link
Contributor

@adrum adrum commented Oct 24, 2020

This product has some custom characteristics to operate as a diffuser, despite being classified as a humidifier. This adds the ability to use these characteristics.

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #23 into main will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
- Coverage   85.04%   85.03%   -0.02%     
==========================================
  Files          45       45              
  Lines        3270     3281      +11     
==========================================
+ Hits         2781     2790       +9     
- Misses        489      491       +2     
Flag Coverage Δ
#unittests 85.03% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ekit/model/characteristics/characteristic_types.py 100.00% <100.00%> (ø)
aiohomekit/model/characteristics/characteristic.py 71.57% <0.00%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91ca4bc...022d6fe. Read the comment docs.

@@ -41,7 +41,10 @@ def filter(self, char_types=None) -> Iterable[Characteristic]:
matches = iter(self)

if char_types:
char_types = [CharacteristicsTypes.get_uuid(c) for c in char_types]
char_types = [
(c if len(c) == 36 else CharacteristicsTypes.get_uuid(c))
Copy link
Owner

Choose a reason for hiding this comment

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

Can we do this normalisation in get_uuid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I originally had it there. I will add it back and update the tests.

Copy link
Owner

Choose a reason for hiding this comment

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

Brilliant, thank you. When that's done I can push a tag/pypi release with it merged for you if you'd like?

@Jc2k
Copy link
Owner

Jc2k commented Oct 24, 2020

Are you planning to add these to HA? There is not really a good story for vendor extensions there yet. I guess the vague plans are:

  • Vendor extensions that can be modelled as standalone sensors and binary_sensors (and then related to the main entity via the device registry) should be. (There was a recent decision that even the "battery charging" attribute should be an entity in its own right. Because of that decision I won't be adding any more attributes without input from HA core devs). This is complicated because right now. There is an assumption of 1 entity per service. So some refactoring will be required. I have a branch in progress for this but it was more work than I had time for at the time.

  • Potentially allow some of the more complicated entity types like climate to be subclassed, loading the subclass if all of its required vendor extensions are detected. This would allow some thermostats the ability to use presets, for example. I do not want to have vendor specific logic in the main entities. Again, refactoring will be needed here.

  • While I am fine with using vendor extensions to implement existing HA interfaces/patterns/services/etc I am really really reluctant to add support for vendor extensions that don't map to anything on the HA side and can only be interacted via vendor specific service calls, especially especially for devices I don't have.

@adrum
Copy link
Contributor Author

adrum commented Oct 24, 2020

@Jc2k I am planning on utilizing this in HA. I have 2 PRs pretty much ready to go, but is related to this PR and utilizes vendor specific chars.

  • The first one is to support a per HAP spec Humidifier/Dehumidifier.

  • The other will be to support this specific VOCOLinc product, as an alternative Humdifier/Dehumidifer. It won't have any vendor specific service calls associated with it, and will work only by utilizing the HA humidifier controls. The PR only leverages one of the chars introduced here, but I went ahead and threw in the rest of the chars.

They both will map to the humdifier entity in HA.

@Jc2k
Copy link
Owner

Jc2k commented Oct 24, 2020

Nice! Sounds like it aligns with my thinking on case 2 then! Great news, i look forward to reviewing those PR's as well!

@Jc2k Jc2k merged commit 0c5241b into Jc2k:main Oct 24, 2020
@Jc2k
Copy link
Owner

Jc2k commented Oct 24, 2020

I've run out of time tonight but I'll try and push out a tag tomorrow.

@adrum
Copy link
Contributor Author

adrum commented Oct 31, 2020

@Jc2k Can you tag a new release? I have the PR ready for this in HA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants