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

Fixed not stopping when moving; Fixed interrupted moving #409

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

sroebert
Copy link
Contributor

@sroebert sroebert commented Feb 8, 2024

After my PR #396 I noticed that my desk is not stopping anymore if pressing stop in Home Assistant. The moving of the desk is also sometimes interrupted. These changes fix these issues, I tested it with Home Assistant and everything seems to be working smoothly again.

I had a local implementation for my own desk which I was using before and the sleep was necessary, but it does not seem to be necessary with the Bleak Bluetooth library.

@coveralls
Copy link

coveralls commented Feb 8, 2024

Coverage Status

coverage: 88.736%. remained the same
when pulling ce486ca on sroebert:bugfix/keep-moving-and-stop
into 01c09f2 on newAM:main.

Copy link
Owner

@newAM newAM left a comment

Choose a reason for hiding this comment

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

I am noticing a lot of hysteresis after these changes.
It is probably something small, but work is keeping me busy these days and I might not have time to debug for a while.

main branch, high to low:

$ poetry run idasen height
0.798 meters
$ poetry run idasen sit
$ poetry run idasen height
0.740 meters

main branch, low to high:

$ poetry run idasen height
0.678 meters
$ poetry run idasen sit
$ poetry run idasen height
0.740 meters

#409, high to low:

$ poetry run idasen height
0.794 meters
$ poetry run idasen sit
$ poetry run idasen height
0.783 meters

#409, low to high:

$ poetry run idasen height
0.683 meters
$ poetry run idasen sit
$ poetry run idasen height
0.694 meters

@sroebert
Copy link
Contributor Author

I will have a look at what is causing this.

@sroebert
Copy link
Contributor Author

Ok, it turned out the delay was still needed when running locally, less so when running via a Bluetooth proxy on esphome. Lowering the delay actually seems to have solved the issues I have seeing. Re-adding the delay also solved the hysteresis. I think it should be good to go now.

abmantis added a commit to abmantis/idasen-ha that referenced this pull request Feb 26, 2024
v0.11.1 introduces some regressions as reported in:
home-assistant/core#110958
newAM/idasen#409
abmantis added a commit to abmantis/idasen-ha that referenced this pull request Feb 26, 2024
v0.11.1 introduces some regressions as reported in:
home-assistant/core#110958
newAM/idasen#409
abmantis added a commit to abmantis/idasen-ha that referenced this pull request Feb 26, 2024
v0.11.1 introduces some regressions as reported in:
home-assistant/core#110958
newAM/idasen#409
@sroebert sroebert requested a review from newAM February 29, 2024 14:56
@sroebert
Copy link
Contributor Author

If you have time to test it with your desk, it would be nice to get this through, idasen-ha was already updated to downgrade to 0.11: home-assistant/core#110958, abmantis/idasen-ha#23

Copy link
Owner

@newAM newAM left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@newAM newAM merged commit 7c39394 into newAM:main Mar 5, 2024
20 checks passed
abmantis pushed a commit to abmantis/idasen-ha that referenced this pull request Apr 4, 2024
newAM/idasen#409 got merged, so the version can
be bumped to 0.12.0 again. This improves the height changes quite a bit,
it will not overshoot or undershoot anymore, but always stop exactly at
the requested height (same as with the IKEA app). Also speed got added,
so once the version is updated, this can also be added as a sensor to
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.

3 participants