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

Feature Request: Cover travel state, travel direction and position while moving from bus instead of travel calculator #1251

Open
LukasGrebe opened this issue Apr 20, 2023 · 3 comments
Labels
💡 feature request Feature request

Comments

@LukasGrebe
Copy link

When I change the position of a cover but the the cover is locked, xknx still reports a traveling cover and false calculated state of the cover.
Some actors such as the MDT JAL actor have an automatic cover travel time sensor built in (based on current or travel time). This method of cover moving state and current position while moving is more accurate as it uses actor internal state information such as locks.

In case of the MDT JAL actor, there are Communication objects available to access the actor calculated Moving State, Moving Direction, and also update the current position while moving, if so configured. See section "4.5.3 Verfahr Status/ aktuelle Richtung" of the Technical documentation (de) as an example.

I'd like to propose adding this to the Cover Device, and offer the option of replacing the TravelCalculator and TravelStatus if GroupAddresses are set.

As far as I can tell the following set of addresses,

        group_address_long: GroupAddressesType | None = None,
        group_address_short: GroupAddressesType | None = None,
        group_address_stop: GroupAddressesType | None = None,
        group_address_position: GroupAddressesType | None = None,
        group_address_position_state: GroupAddressesType | None = None,
        group_address_angle: GroupAddressesType | None = None,
        group_address_angle_state: GroupAddressesType | None = None,
        group_address_locked_state: GroupAddressesType | None = None,

would need additional
group_address_travel_status: GroupAddressesType | None = None,
and optionally
group_address_travel_direction: GroupAddressesType | None = None,

with current position intermittently updated via group_address_position_state instead of the calculator.

I'm not sure how a conditional use of the travel calculator and travel state would be set up.

@LukasGrebe LukasGrebe changed the title Feature Request: Traveling State & Position from Bus Updates instead of calculated. Feature Request: Cover travel state, travel direction and position while moving from bus instead of travel calculator Apr 20, 2023
@farmio farmio added the 💡 feature request Feature request label Apr 20, 2023
@farmio
Copy link
Member

farmio commented Apr 22, 2023

Hi 👋!
I don't think travel calculator and position updates are mutually exclusive.

Travel calculator uses position updates as new known position and resets the update timer on reception. So if there is a position update every second, the travel calculator is effectively disabled. If there are updates every 5 seconds while traveling it would add 4 intermediate callbacks.

For travel direction we have current position and new position or the move trigger telegram. So I guess we don't really need a new GA to determine the direction.

What's left is an address for currently moving. If we check this to start our moving-methods (instead of long / short), I think it would work properly.

We also have ga_locked - this could also be used to suppress moving-methods. It could also set the HA entity to "unavailable" so it can not be triggered from HA UI when it wouldn't react anyway.

@LukasGrebe
Copy link
Author

interesting points!
a current workaround: i set the moving time in HA to 1s. so the updates coming in overwrite any calculation.

theoreticaly, if I set it to the correct run time and considering your explanations of intermittent updates, it should result in a perfect representation. With the exception of the locked case where HA will show a moving cover that isn't actually moving.
maybe a ga_locked pull request would be better placed with the KNX HA integration at https://github.com/home-assistant/core/tree/dev/homeassistant/components/knx

@farmio
Copy link
Member

farmio commented Apr 25, 2023

a current workaround: i set the moving time in HA to 1s. so the updates coming in overwrite any calculation.

Incoming position updates always overwrite any calculation.

Downside of setting wrong (eg. 1s) travel times is that delay deviation (eg. from network latency) could cause xknxs 1s to trigger some milliseconds before the actuators update (if it updates every second - which not every actuator does) which causes 2 update callbacks to HA

  • first for the calculated (wrong) travel time with open/closed state - as the full travel time has already passed
  • second with position update from the actuator

When this happens (xknxs 1 second triggering faster than the bus update is being processed) the travel calculator will assume the cover is not moving anymore, because the target position was already reached (calculated). A bus position update will then only update the position, but the travel state will not be set "opening" or "closing" anymore because technically we don't know the direction of movement - or even if it is moving.

With the (at least almost) correct travel time, 2 update callbacks can of course also happen, but they would send an almost identical state so this should be barely noticeable.

it should result in a perfect representation

Unless you run SMI or something like that, the actuator doesn't do anything different than what we are doing. It only calculates intermediate positions based on time. If you had not the correct travel time configured in the actuator or a motor with soft-start/stop the bus position would also slightly deviate from reality. So all we can aim for is a perfect representation of the internal state of the actuator.

maybe a ga_locked pull request would be better placed with the KNX HA integration

Currently we set all entities to "unavailable" state if the connection drops. Having disconnected and locked result in "unavailable" could be confusing. I guess disconnected should be represented differently at some point. Or someone takes a stab at extending HAs cover entity capabilities for "read only" or something like that. See home-assistant/architecture#545

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

No branches or pull requests

2 participants