Added remote entity in addition of media player entity#90
Added remote entity in addition of media player entity#90albaintor wants to merge 36 commits intounfoldedcircle:mainfrom
Conversation
…abled will replace volume up/down and mute commands
…upported (all profiles actually)
|
I have tested it successfully : both entities work correctly on my side so far |
|
Great, thanks for the enhancement! This might take some time to test, but I try to speed it up. |
zehnm
left a comment
There was a problem hiding this comment.
This looks good! Only a few minor things I found in the code review.
Next will be testing it on the Remote :-)
Don't worry about the repeat & sequence handling. We can simply say it's for shorter sequences with just a few commands. Anything else requires a macro in the Remote.
Note: the upcoming "press-and-hold" feature should have some support in the integration-library to simplify integration drivers, otherwise the same async processing logic needs to be put in every driver.
|
Just applied a math calculation to evalutate expected duration of command or command sequence |
|
I have found a better way finally : with |
|
Ok this is over I fixed a minor thing and linting |
|
I'll create another test release from the main branch to test the updated Python & pyinstaller versions, then come back to the remote-entities in Android & Apple TV. |
|
It's best not to update the driver.json metadata in PRs to avoid merge conflicts. I update it when creating a release. |
zehnm
left a comment
There was a problem hiding this comment.
Looks good, if something comes up during testing, we can always fix it later :-)
After fixing the merge conflict I'll merge it.
|
Hi @zehnm I made tests and I fixed several things (wrong unit for delay, parameters not correctly parsed for command sequence) I also updated androidtvremote2 : no impacts on what we are using but a new command to send text for future use maybe |
zehnm
left a comment
There was a problem hiding this comment.
Sorry for the delay, the wifi firmware release took away way more time than anticipated.
I've fixed the linting issue and did a quick remote-entity test with the Nvidia ShieldTV.
Normal commands work great, but unfortunately I found a show stopper preventing this PR from merging.
With the default volume handling (not using the Chromecast feature) and an AVR (HDMI), the volume commands don't stop after releasing the button on the remote. Pressing a volume up/down button on the remote for 2 seconds will continue increasing/decreasing volume for another ~6 seconds. Using the mediaplayer-entity for volume control it's "only" about one additional second that the volume changes (about 2 volume steps on my Denon AVR). If I remember correctly, the media player entity behaviour has been like that before.
Checking the logs reveals the following remote-entity behaviour:
DEBUG:ucapi.api:[('192.168.16.34', 47420)] <-: {"kind":"req","id":610,"msg":"entity_command","msg_data":{"cmd_id":"send_cmd","entity_id":"remote.00044BFAEA10","entity_type":"remote","params":{"command":"volume_down","repeat":4}}}
INFO:remote:[remote.00044BFAEA10] Got command request: send_cmd {'command': 'volume_down', 'repeat': 4}
DEBUG:androidtvremote2:Sending: remote_key_inject { key_code: KEYCODE_VOLUME_DOWN direction: SHORT }
DEBUG:androidtvremote2:Sending: remote_key_inject { key_code: KEYCODE_VOLUME_DOWN direction: SHORT }
DEBUG:androidtvremote2:Sending: remote_key_inject { key_code: KEYCODE_VOLUME_DOWN direction: SHORT }
DEBUG:androidtvremote2:Sending: remote_key_inject { key_code: KEYCODE_VOLUME_DOWN direction: SHORT }
DEBUG:ucapi.api:[('192.168.16.34', 47420)] ->: {'kind': 'resp', 'req_id': 610, 'code': 200, 'msg': 'result', 'msg_data': {}}
DEBUG:ucapi.api:[('192.168.16.34', 47420)] <-: {"kind":"req","id":611,"msg":"entity_command","msg_data":{"cmd_id":"send_cmd","entity_id":"remote.00044BFAEA10","entity_type":"remote","params":{"command":"volume_down","repeat":4}}}
INFO:remote:[remote.00044BFAEA10] Got command request: send_cmd {'command': 'volume_down', 'repeat': 4}
DEBUG:androidtvremote2:Sending: remote_key_inject { key_code: KEYCODE_VOLUME_DOWN direction: SHORT }
DEBUG:androidtvremote2:Sending: remote_key_inject { key_code: KEYCODE_VOLUME_DOWN direction: SHORT }
DEBUG:androidtvremote2:Sending: remote_key_inject { key_code: KEYCODE_VOLUME_DOWN direction: SHORT }
DEBUG:androidtvremote2:Sending: remote_key_inject { key_code: KEYCODE_VOLUME_DOWN direction: SHORT }
DEBUG:ucapi.api:[('192.168.16.34', 47420)] ->: {'kind': 'resp', 'req_id': 611, 'code': 200, 'msg': 'result', 'msg_data': {}}
Each single volume_down command sends 4 individual KEYCODE_VOLUME_DOWN key presses.
The repeat parameter for a remote-entity indicates a repeat sequence that needs to be extended if the same command is received again and is still active.
This works well for IR entities with native IR-repeat.
For IP controlled devices this can be a challenge and only works if there's a HID report structure or individual "start press" and "stop press" commands.
The upcoming "press-and-hold" feature will address that issue and makes it a bit easier handling long presses vs send command x times (for example in a macro).
Fortunately the Android TV remote service has direction = "START_LONG" and END_LONG parameters in send_key_command. This will make it a true long press as with the original remote, also independent from the key repeat rate of the UC Remote.
Intended logic for long press key behaviour (untested):
- If a key repeat (press-and-hold) is detected: send a single
direction = "START_LONG". - For each repeat command received within a defined key-press timeout: extend button press (by doing nothing)
- If key-press timeout or the new stop_send command is received: send a single
direction = "END_LONG".
I wouldn't be surprised if we find some quirks. The START_LONG might also automatically timeout and not be active for minutes. That needs to be analysed, also if it auto-resets if another key is sent.
As a quick-and-dirty workaround the repeat count could be reset in remote.py until the new feature is implemented:
async def send_commands(self, cmd_id: str, params: dict[str, Any] | None = None) -> StatusCodes:
"""Handle custom command or commands sequence."""
# hold = self.get_int_param("hold", params, 0)
delay = get_int_param("delay", params, 0)
repeat = get_int_param("repeat", params, 1)
# temporary hack for hold-down buttons sending a repeat count.
# This will be addressed with the upcoming press-and-hold feature.
if repeat == 4:
repeat = 1If that's ok with you I could continue the implementation in this PR towards the end of October and add the new repeat feature? I'll be travelling and away for the next weeks,
|
Do you think that a "true long" press using AndroidTV No problem for waiting when you're ready. Can you update the UC library to prepare the other integrations ? I am not sure many devices support long press (Zidoo does I think) anyway One last thing to consider with these start/stop long press : the Do you want me to implement this new long press mode ? |
It's my expectation that this works :-) I'm finally back home again with access to the Android TV devices to test it today or tomorrow. I'll report back. |
|
Forgot to report back: technically it "works", but it's not usable. The longer the {
"command_map": {
"fast_forward": {
"keycode": "KEYCODE_VOLUME_DOWN",
"action": "BEGIN"
},
"rewind": {
"keycode": "KEYCODE_VOLUME_DOWN",
"action": "END"
}
}
}The only option I see right now is to write a custom repeat handler that sends individual commands at a certain rate, when the long-press addition becomes available in the integration-API. |
You mean that after sending STOP_LONG, the command remains active during the "pressed" duration, wheach leads to twice the pressed duration ? |
|
The command continues to be active for a certain time after the |
zehnm
left a comment
There was a problem hiding this comment.
I have started with the voice assistant implementation in the feat/voice branch, where I also require to expose a new entity per Android TV device.
Your enhancements in this PR, mainly driver.py and media_player.py, were a big help to start the refactoring.
The following comments are not meant for you to change your PR! Most of them are already included in my feature branch :-) Furthermore, I've prepared the remote-entity handling in driver.py to simplify adding the remaining remote-entity logic in remote.py.
After the Android-TV voice addition, the press-and-hold feature for remote-entities will be finished, so we can finalize this PR.
| self._device_config = device_config | ||
| self._profile = profile | ||
|
|
||
| entity_id = create_entity_id(device_config.id, EntityTypes.MEDIA_PLAYER) |
There was a problem hiding this comment.
Changing the entity identifier of the existing media-player entity is a serious breaking change. All existing user configurations won't work anymore and have to be redone (including activities, macros).
We've made this mistake in the Denon integration and can't repeat it for the Android TV integration, especially since there are more Android TV users.
There was a problem hiding this comment.
Changed logic in feat/voice branch: original media-player entity remains as is, other entities get the entity-type prefix.
| """ | ||
| # dead simple for now: one media_player entity per device! | ||
| # TODO #21 support multiple zones: one media-player per zone | ||
| return [f"media_player.{device_id}", f"remote.{device_id}"] |
There was a problem hiding this comment.
See comment in media_player.py: existing entity may not be renamed
There was a problem hiding this comment.
Changed logic in feat/voice branch
| config.devices.update(device) | ||
|
|
||
| # TODO is this the correct state? | ||
| api.configured_entities.update_attributes(identifier, {MediaAttr.STATE: ucapi.media_player.States.STANDBY}) |
There was a problem hiding this comment.
Remove old logic: entity states have been set already above
| if configured_entity.entity_type == ucapi.EntityTypes.MEDIA_PLAYER: | ||
| if ( | ||
| configured_entity.attributes[ucapi.media_player.Attributes.STATE] | ||
| == ucapi.media_player.States.UNAVAILABLE | ||
| ): | ||
| # TODO why STANDBY? | ||
| api.configured_entities.update_attributes( | ||
| entity_id, {ucapi.media_player.Attributes.STATE: ucapi.media_player.States.STANDBY} | ||
| ) | ||
| else: | ||
| api.configured_entities.update_attributes( | ||
| entity_id, {ucapi.media_player.Attributes.STATE: ucapi.media_player.States.ON} | ||
| ) | ||
| elif configured_entity.entity_type == ucapi.EntityTypes.REMOTE: | ||
| if configured_entity.attributes[ucapi.remote.Attributes.STATE] == ucapi.remote.States.UNAVAILABLE: | ||
| api.configured_entities.update_attributes( | ||
| entity_id, {ucapi.remote.Attributes.STATE: ucapi.remote.States.OFF} | ||
| ) |
There was a problem hiding this comment.
This will require a bit more analysis, but I think the best approach is to set the entity state to UNKNOWN after connection if the current state is UNAVAILABLE (set in handle_disconnected). Right after this callback, there should be one or multiple device update events containing the device state. If that's the case, this logic can be simplified.
- the real state of the device should be fetched.
- media-player & remote don't seem to be in sync (ON vs OFF)
There was a problem hiding this comment.
Simplified logic in feat/voice branch: will do further tests
|
|
||
| configured_entity = api.configured_entities.get(entity_id) | ||
| if configured_entity is None: | ||
| configured_entities = _entities_from_device_id(atv_id) |
There was a problem hiding this comment.
_entities_from_device_id returns a hardcoded list of all possible entity identifiers of the device. But that is not the list of configured entities! A user can choose to just configure one entity.
If not all possible entities were configured, every call generates misleading entity not found log entries.
- rename
configured_entities:device_entities - remove "not found" log statement in the get method in the integration library (recently done in the feat/voice branch)
Create a new helper function to retrieve a list of configured entities of a device identifier. The following code block is duplicated a few times:
for entity_id in _entities_from_device_id(identifier):
configured_entity = api.configured_entities.get(entity_id)
if configured_entity is None:
continueThere was a problem hiding this comment.
Changed logic in feat/voice branch: added _configured_entities_from_device and _configured_entity_ids_from_device helper functions
| if isinstance(configured_entity, media_player.AndroidTVMediaPlayer): | ||
| old_state = ( | ||
| configured_entity.attributes[MediaAttr.STATE] | ||
| if MediaAttr.STATE in configured_entity.attributes | ||
| else ucapi.media_player.States.UNKNOWN | ||
| ) | ||
|
|
||
| if MediaAttr.STATE in update and update[MediaAttr.STATE] != old_state: | ||
| attributes[MediaAttr.STATE] = update[MediaAttr.STATE] | ||
|
|
||
| if MediaAttr.MEDIA_TITLE in update: | ||
| attributes[MediaAttr.MEDIA_TITLE] = update[MediaAttr.MEDIA_TITLE] |
There was a problem hiding this comment.
Move state logic to AndroidTVMediaPlayer as it was done for AndroidTVRemote
| configured.disconnect() | ||
|
|
||
|
|
||
| async def connect_device(device: tv.AndroidTv): |
There was a problem hiding this comment.
Is connect_device used anywhere? I can't find a reference
| attributes = { | ||
| MediaAttr.STATE: self._player_state, | ||
| MediaAttr.MUTED: self._muted, | ||
| MediaAttr.MEDIA_TYPE: self._media_type, |
There was a problem hiding this comment.
Invalid media type: initialized to METADATA_TYPE_MOVIE from pychromecast, which is a numeric value 1.
I'll fix this in my PR #120 where I copied over this code.
| if MediaAttr.STATE in configured_entity.attributes | ||
| else media_player.States.UNKNOWN | ||
| ) | ||
| for entity_id in configured_entities: |
There was a problem hiding this comment.
There seem to be much more entity updates now containing the same data as in the last event, especially when Chromecast is enabled.
I see up to 6 entity_change events per second with the same data!
This generates > 20 log entries per second for the integration alone, which is too much for an embedded device.
I'll try to fix this in my open PR #120, since I've copied over that code
Support Google voice commands. The Android TV device needs to support voice commands. This is returned as a feature flag after connection. This is a preview feature and needs to be enabled in the setup device configuration. Closes #87 Other changes: - Refactor entity handling in driver.py to support multiple entities per Android TV. Based on the remote-entity PR #90 - use ucapi 0.5.1 - Fix default media type and media information handling if Chromecast is enabled. - Fix log and entity state update flooding when using Chromecast feature
Hi,
I have added support for remote entity in addition of media player.
I had to make heavy modifications in the code so a lot of tests should be done to check after regressions