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 function to get limitation for windows with rain sensor #71

Merged
merged 6 commits into from
Sep 6, 2021

Conversation

mago0211
Copy link

@mago0211 mago0211 commented Jul 19, 2021

Add support to get limitations.
Limitations can currently read only on request (poll). There is not status notifications from Gateway on change.

Fixes: #67

@mago0211
Copy link
Author

Hi @Julius2342,
is something wrong with this PR?
@pergolafabio had test the Code and it works.

Thanks and Regards
Markus

@pergolafabio
Copy link

indeed, still runnning and no issues with it

@Julius2342
Copy link
Owner

There are conflicts, may I ask you solve them?

@mago0211
Copy link
Author

There are conflicts, may I ask you solve them?

Done

@pergolafabio pergolafabio mentioned this pull request Aug 24, 2021
@Julius2342
Copy link
Owner

Before i review, some checks are failing: https://github.com/Julius2342/pyvlx/pull/71/checks?check_run_id=3414537725#step:7:222

@mago0211
Copy link
Author

Yes, fixed checks https://github.com/mago0211/pyvlx/runs/3415304542

@codecov-commenter
Copy link

Codecov Report

Merging #71 (7db29bd) into master (00e5989) will increase coverage by 0.69%.
The diff coverage is 94.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   81.79%   82.49%   +0.69%     
==========================================
  Files          73       75       +2     
  Lines        2967     3090     +123     
==========================================
+ Hits         2427     2549     +122     
- Misses        540      541       +1     
Impacted Files Coverage Δ
pyvlx/opening_device.py 54.66% <28.57%> (-2.69%) ⬇️
pyvlx/pyvlx.py 58.49% <50.00%> (-1.51%) ⬇️
pyvlx/api/frame_creation.py 97.61% <100.00%> (+0.11%) ⬆️
pyvlx/api/frames/__init__.py 100.00% <100.00%> (ø)
pyvlx/api/frames/frame_get_limitation.py 100.00% <100.00%> (ø)
pyvlx/api/get_limitation.py 100.00% <100.00%> (ø)
pyvlx/api/api_event.py 54.83% <0.00%> (+19.35%) ⬆️

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 00e5989...7db29bd. Read the comment docs.

@mago0211
Copy link
Author

Hi,
i don't have access to detail coverage report. I run coverage measurement local and see in
pyvlx/opening_device.py
async def get_limitation(self): get_limitation = GetLimitation(pyvlx=self.pyvlx, node_id=self.node_id) await get_limitation.do_api_call() if not get_limitation.success: raise PyVLXException("Unable to send command") return get_limitation

has no coverage but I am undecided how to test this? Yes i can create mocks but makes this sense here?

The same in
pyvlx/pyvlx.py
async def get_limitation(self, node_id): limit = get_limitation.GetLimitation(self, [node_id]) await limit.do_api_call()

@Julius2342
Copy link
Owner

Hmm, good question. If nothing helps, mocking is always an option.

(The reasons for unit tests are not only covering complex scenarios or e.g. verification of parsing results, it is also checking for typos etc.)

I had a first look at the PR, it looks sane. Will do a line by line review later ... Thank you very much :-)

@Julius2342 Julius2342 merged commit 76f624a into Julius2342:master Sep 6, 2021
@Julius2342
Copy link
Owner

Thank you very much :-)

@mago0211 mago0211 deleted the add_limitations branch September 9, 2021 07:17
@pergolafabio
Copy link

hey @mago0211 , are you still working on this? to make it available in HA?

thnx

@mago0211
Copy link
Author

Hi,
I started some work for HA but i am currently to busy. Maybe in the next Holiday i can spend some time for it.
What we need in any case is a new Release of pyvlx @Julius2342 do you have a plan for a new release?

@pergolafabio
Copy link

Bump :-)

@Julius2342
Copy link
Owner

Hey, in case you did not see - i released a new version ...

@pergolafabio
Copy link

cool, thnx for the feedback!!

appreciatied

@mago0211 , keep me posted if you start implementing this :-)

@pergolafabio
Copy link

Ok, updated my manifest to 0.2.20, no errors in my custom script, thnx!

@mago0211
Copy link
Author

@pergolafabio
fyi
I started some work for the limitation sensor in HA.
mago0211/core@7e82744

Currently i have some crazy issues when fetching data but basically it works.
I hope that I can finish it the next few days, there are still things to do.

@pergolafabio
Copy link

Cool, will try out tomorrow...
This will give an extra attribute for each cover? With the value 0 or 93 ?

@pergolafabio
Copy link

What are the crazy things then?

@pergolafabio
Copy link

ok, have it running
the scan interval is now hardcoded to 300 ? will the scan interval in yaml override it?

image

@mago0211
Copy link
Author

I have something updated
mago0211/core@c3930da

you can now set the update interval via config in seconds (Default is 300)

velux:
host: xxx
password: xxx
scan_interval: 10

Cool, will try out tomorrow... This will give an extra attribute for each cover? With the value 0 or 93 ?

Yes
0 == NoRain
93 == Rain

What are the crazy things then?

The gatway comes into trouble if you fetch limitation data for too many windows in parallel. This is now prevented by the integration.

@pergolafabio
Copy link

Ok , gonna update tomorrow , thnx for scan interval, is indeed usefull...
Didn't notice the issue, bur my script was only fetching one window each 180 sec

@mago0211
Copy link
Author

@pergolafabio
There is an open PR in HA repo home-assistant/core#64628

@pergolafabio
Copy link

nice!! maybe in next release :-)

good job!!

@mago0211
Copy link
Author

@pergolafabio
Have you noticed the comment on PR home-assistant/core#64628 (comment)
I would set a default value for and make it non-configurable. 300Seconds?
I have currently no time to switch the integration to config_flow otherwise i must close the PR in HA.

@pergolafabio
Copy link

I, I saw it, maybe make it 120?

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.

Rain Sensor
4 participants