Multiple instances branch

[albaintor]

Hi Markus,

I have developed the multiple instances based on previous work but this time in a dedicated branch (without the additional commands) -> the tricky thing for Android is the certificate & key files to be splitted.
As we don't know the mac address (which is the ID of the device) while connecting, the pair of files will be generated without the id in the name, and after pairing I renamed them with the macaddress inside the name.
This works on my side, but maybe a better/prettier solution could be found

Closes #14

[albaintor]

Otherwise @zehnm I checked after your branch on additional commands, there is something missing : the context menu feature is not in the list, so the command won't be available (and it simulates the long press ENTER so it is very useful)
To be added in profiles.py line 168 => media_player.Features.CONTEXT_MENU
EDIT : + modification of profiles/NVIDIA_SHIELD.json => add "context_menu"

Also there is another PR with the addition of Kodi that is working on my side : to be added in apps.py line 24
"Kodi": {"url": "market://launch?id=org.xbmc.kodi"},

Thank you for the hard work

[zehnm]

I will check this after PR #34 is merged

[zehnm]

To start a code review please sync with main branch to resolve merge conflicts (from recently merged PR 34) and fix the linting issues first.

[albaintor]

To start a code review please sync with main branch to resolve merge conflicts (from recently merged PR 34) and fix the linting issues first.

Done

[albaintor]

Please note : as I changed the name of certificate file names to handle multiple instances, in driver.py when it starts, first I check after all the certificate files if they exist, otherwise I rename them (done once).
Then in the setup flow the certificate files are renamed on the fly after successful pairing

[zehnm]

This is just an initial code review, I'll check the functionality and certificate handling tomorrow.

Please also check and fix the linting issues. See https://github.com/unfoldedcircle/integration-androidtv/blob/main/docs/code_guidelines.md#tooling on how to check locally.

[albaintor]

Done

[zehnm]

The setup flow is currently broken due to invalid certificate path and a wrong comparison of already configured devices (see source comments). Please fix this, then I can merge it.

I'd like to simplify the certificate handling and centralize the filename & rename logic, but I can refactor this myself after this PR is merged. For now it's more important that the multi-device handling is working :-)

Currently the logic is spread out in tv.AndroidTv, setup_flow.handle_user_data_pin and driver.main For example using config.Devices.migrate for migrating old certificates, instead of doing it in main(), and simply passing the certfile & keyfile to use to AndroidTV. I'll investigate it further.

[albaintor]

The setup flow is currently broken due to invalid certificate path and a wrong comparison of already configured devices (see source comments). Please fix this, then I can merge it.

I'd like to simplify the certificate handling and centralize the filename & rename logic, but I can refactor this myself after this PR is merged. For now it's more important that the multi-device handling is working :-)

Currently the logic is spread out in tv.AndroidTv, setup_flow.handle_user_data_pin and driver.main For example using config.Devices.migrate for migrating old certificates, instead of doing it in main(), and simply passing the certfile & keyfile to use to AndroidTV. I'll investigate it further.

I am on it...

[albaintor]

The setup flow is currently broken due to invalid certificate path and a wrong comparison of already configured devices (see source comments). Please fix this, then I can merge it.

I'd like to simplify the certificate handling and centralize the filename & rename logic, but I can refactor this myself after this PR is merged. For now it's more important that the multi-device handling is working :-)

Currently the logic is spread out in tv.AndroidTv, setup_flow.handle_user_data_pin and driver.main For example using config.Devices.migrate for migrating old certificates, instead of doing it in main(), and simply passing the certfile & keyfile to use to AndroidTV. I'll investigate it further.

I don't understand this comment :
in driver.py I added the following code to migrate the current certificate with "old" naming, in addition of the renaming during the setup flow for new devices. So current devices should have files renamed when starting the driver (which should occur after firmware update), and new devices will get benefits of the new naming.
I agree with the rest of your comment though : I don't like this renaming process. The best would be to include the certificate strings directly in the json and to push them as strings to the library.

    for device in config.devices.all():
        # Migration of certificate/key files with identifier in name
        _android_tv = tv.AndroidTv(api.config_dir_path, device.address, device.name, device.id)
        if not os.path.exists(_android_tv.certfile):
            current_certfile = os.path.join(api.config_dir_path, "androidtv_remote_cert.pem")
            current_keyfile = os.path.join(api.config_dir_path, "androidtv_remote_key.pem")
            try:
                _LOG.info("Rename certificate file %s to %s", current_certfile, _android_tv.certfile)
                os.rename(current_certfile, _android_tv.certfile)
                _LOG.info("Rename key file %s to %s", current_keyfile, _android_tv.keyfile)
                os.rename(current_keyfile, _android_tv.keyfile)
            except OSError as ex:
                _LOG.error("Error while migrating certificate files: %s", ex)

        _add_configured_android_tv(device, connect=False)

EDIT : ok you were mentioning the other bugs you listed above and that I just fixed. This is okay (for now) ?

[zehnm]

I don't understand this comment :
in driver.py I added the following code to migrate the current certificate with "old" naming :

Don't worry, leave it as is and I can refactor it later so that all certificate file handling logic including migration is done in one place.
The device identifier might be used from the mDNS service discovery: TXT record bt. At least for Chromecast & Shield. But I have no idea if that's always present for all Android TV devices like Philips and Sony TVs. That would need further testing. At least for my streaming devices it's the same Bluetooth MAC identifier as in the certificate, which stays the same no matter if the device is using a LAN or WiFi connection.

I'll check your fixes now.

[zehnm]

Almost there! One linting issue and workaround to restore in the setup flow.
Then it's ready to merge.

[albaintor]

Ok committed !

[zehnm]

Looking good, thanks for your contribution!

I'll probably refactor the certificate handling or storage location of the certificate files before the next release.

[zehnm]

This will require more fixes, the certificate file migration doesn't work from the last release version running on the remote. I'll create a new issue and fix it.

[albaintor]

This is strange it worked in the simulator but with an external driver