-
Notifications
You must be signed in to change notification settings - Fork 70
Conversation
binary_sensor. to do: - see to it a prefix 'hue_' is added to the device_id (currently the binary_sensor.daylight is created with friendly_name Hue Daylight) - take care of multiple Hue Hubs, creating identical sensors. A unique (Hub) Id should be added to the device_id
OK I want to start adding tests for the |
I'd need an example how to do this, so if you would 'get the ball rolling'... |
OK I've added test framework |
not really understanding what to do now.. I've got my PHD sensor running just fine, not sure we need an extra test for that? do you simply create a mockup sensor with a fake parsed state? also, eg the mockup temperature sensor shows all attributes, but the parsed temperature only has a state? I guess I could simply c&p this, but need some feedback on the why, and the above what/how... |
Just require a test for |
ok, so let me try: this I the data returned by the API:
and the parsed_phd could then be (also based on the config I made for the PHD sensor like this?:
and add to the def test:
hope this approaches what you need... |
Correct, but note that the returned data is already being pre-processed into a python dict, by the only effect is changing |
ok, have it setup but I fear it will add a new PR, instead of adding to this... How can I add another file to change to this PR? |
Sorry I dont understand. Please add to this PR |
my bad, really sorry.. How do I add another file to change to the same (this) PR? If I simply browse to the tests file in the repo and click edit, it creates a new PR, and doesn't add to this PR. |
You need to rebase. This is quite fundamental stuff to understand so please take your time and make sure you are confident you understand it all |
CHG: make daylight sensor unique per bridge
Pull Request Test Coverage Report for Build 45
💛 - Coveralls |
well, I feel somewhat inadequate here.. also, since I can't find a real tutorial on these issues, I fear I must let go, until someone could instruct me. which truly is a pity, since Ive got the pr working flawlessly on my local instance. Ive accidentally stumbled onto the @koying pr, (thanks for that) and merged it, but there's an 'uncovered' line there, which seems perfectly fine to my eyes. check fails on it though.. so 2 issues left: the @koying pr wont merge just yet |
I saw you created tests in a seperate PR, they should be in this one |
yes, I understand that... I simply don't know how to do that. I truly am trying to learn and read, unfortunately the GitHub ecosystem is less straightforward than I would have hoped. Meanwhile, the @koying PR works as expected, adding the unique hub id to the sensor, very nice indeed, thank you very much! |
It is a learning curve we must all go through, but it is worth it! |
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
=========================================
Coverage ? 14.24%
=========================================
Files ? 3
Lines ? 386
Branches ? 0
=========================================
Hits ? 55
Misses ? 331
Partials ? 0
Continue to review full report at Codecov.
|
that might well be the case, but since no help is available, or no one is willing to point me in the correct direction, I think this must be it. The completely incomprehensible coverage/coveralls issue which has been newly added to this only further complicates things. I'll resort to my locally working files, and please feel free to adopt that. |
} | ||
|
||
|
||
def parse_hue_api_response(sensors): | ||
def parse_hue_api_response(bridgeid, sensors): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you passing bridgeid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To build unique names for the sensor when multiple bridges are involved.
There is not "unique_id" for that sensor...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would also apply to remotes etc then. This should be applied consistently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean.
All the "actual devices" have their own unique_id, assumed to be unique across bridges:
"uniqueid": "00:17:88:01:10:4f:75:9b-0b"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to the above , the daylight sensor is a per hub sensor, and not a Hue.
accessory
We need the hub id because people have multiple hub setups
This sensor doesn’t have a unique id like the others because it is bound to the hub. So the hub id needs to be set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I do see the benefit. However I do want to find a better way to handle the multiple hue hub issue. For example, the hue hub id could be an attribute on all the sensors on that hub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, I am using it right now
Well sure, but the question is, in the context of HA, why you're using that one rather than "sun"?
There is a bunch of additional attributes in "sun" (https://www.home-assistant.io/integrations/sun/) and it's handled specifically as far as automations are involved, ie you can specify offsets in the trigger (https://www.home-assistant.io/docs/automation/trigger/#sun-trigger) while the hue offsets can only be altered with the hue app.
The only config needed is your geographic location in HA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh, since the introduction of native Hue sensors in HA, one could ask this question Why, on most of the entities this CC creates...
the answer is always: because it is better, more flexible, quicker etc etc.
In this particular case: I mimic the Hue setup in my HA config. The one sensor missing was the daylight setting in the Hub.
This will allow for further logic in HA to use in home/away schedules, and the options the App offers (Hue is expanding that at the moment, given the extra rules being created)
I am aware of what we can do in the HA ecosystem, like offsets in triggers etc, but you might also know that exactly that subject is cause for great confusion in the community.
Having a clear daylight sensor being on/off following Hue rules is very very adequate and easy to use. No more confusions which (light, day, dark, above_horizon,offset)sensor to use to mimic Hue behavior. Simply use hue_daylight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK my decision is that at this time there is not a strong case for inclusion - each line of code adds a burden of maintainence and possibility for bugs. @Mariusthvdb please create a feature request (citing this PR) highlighting why this is substantially different from the sun sensor. If there are several people keen then I will consider again. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider that done: thanks for the opportunity!
#212
About the prefix (hub id) on the daylight sensor:
that could be altered to use the hub_id also. Not necessarily necessary.... (since it already has a uniqueid,) but truly consistent. |
As title
still to do: