Provide VIN to callback(s) registered with subscribe_updates()

[Giermann]

See issue #501 for details.

[dvx76]

Thanks for your contribution!

Couple of notes if we change this:

• Should prob be marked as breaking, since we change the expected signature of callback functions
• Will need a corresponding update in homeassistant-myskoda: https://github.com/skodaconnect/homeassistant-myskoda/blob/a00f9b7ad90875ab9cca687541cee662284d81fb/custom_components/myskoda/coordinator.py#L216

In homeassistant-myskoda there's a separate coordinator per vehicle/VIN, which is why this isn't a problem there. It already knows the vin.

[Giermann]

* Should prob be marked as breaking, since we change the expected signature of callback functions

I'm no python expert at all, but isn't the code backward compatible, if existing callbacks simply ignore the now provided parameter?

* Will need a corresponding update in homeassistant-myskoda: https://github.com/skodaconnect/homeassistant-myskoda/blob/a00f9b7ad90875ab9cca687541cee662284d81fb/custom_components/myskoda/coordinator.py#L216

Same here: if registering a callback that takes no parameter is still working, nothing needs to be changed...

[WebSpider]

* Should prob be marked as breaking, since we change the expected signature of callback functions

I'm no python expert at all, but isn't the code backward compatible, if existing callbacks simply ignore the now provided parameter?

No, python enforces the parameter to be of type Vin now.

* Will need a corresponding update in homeassistant-myskoda: https://github.com/skodaconnect/homeassistant-myskoda/blob/a00f9b7ad90875ab9cca687541cee662284d81fb/custom_components/myskoda/coordinator.py#L216

Same here: if registering a callback that takes no parameter is still working, nothing needs to be changed...

Same answer :)

[Giermann]

No, python enforces the parameter to be of type Vin now.

Okay, then it might be easier to introduce a separate subscribe_vehicle_updates().
But this would need another list of callbacks. But this list could be global, not bound to a specific VIN, making the additional parameter vin in subscribe_updates() obsolete.

It's currently not clear to me, why this design is different to subscribe_events(). An event containing the VIN is being passed to a global list of callbacks there. For updates there are different callback lists for each vehicle that do not receiving any reference to the VIN that issued the call.

I guess this needs some discussion to find the best solution.

[Giermann]

After looking deeper into the current implementation and thinking about possible solutions and namings, I'd prefer the following:

As I still not know, why there are VIN-specific callback lists, one solution would be to report vehicle updates the same way, as MQTT Events are being reported. One global list of callbacks, that receive some "BaseEvent" as parameter.
MQTT events are already being reported as ServiceEvent, OperationEvent and so on.

In future the same infrastructure could be used to report an instance of UpdateEvent or more specifically VehicleUpdateEvent.
These could be implemented as VehicleUpdateEventData, VehicleUpdateEventInfo, VehicleUpdateEventCharging, ...

For backward compatibility, VIN specific lists could still be registered with subscribe_updates().
All existing callbacks registered with subscribe_events() should already be able to identify the correct instance of BaseEvent.
What do you think?

[WebSpider]

I had a look at this piece of the code, since it's been a while :)

(please disregard my previous remark)
The main difference between the two, is that the events are handled by the mqtt client exclusively, where the updates are handled internally.

We keep more information than what we can obtain from mqtt, so that's the root of the difference.

Now, to address your issue. If i understand correctly, you are missing information when processing the callback, as it could be related to multiple Vins.

Why not let the called function handle that like this?

myskoda.subscribe_updates(myvin, myasyncfunc(vin=myvin))

[dvx76]

Why not let the called function handle that like this?

myskoda.subscribe_updates(myvin, myasyncfunc(vin=myvin))

I don't think that works because subscribe_updates expects a callable in the second argument. So you'd need to create a callback function (with no arguments) for each vin. Since there are no async lamba functions (yet, apparently) something like this should work. But is kinda ugly I guess.

def make_callback(vin: str):
    async def callback():
        await your_vin_specific_callback_function(vin)
    return callback

for vin in list_of_vins:
    myskoda.subscribe_updates(vin, make_callback(vin))

That being said, I think it might make sense for the callback to include the VIN for which there is an update. Again in homeassistant-myskoda we don't have this problem because we have a dedicated coordinator object per VIN, so it already knows which VIN the update is for. But it's reasonable to have myskoda clients which handle multiple VINs from the same place. And then it makes sense for the callback to include the actual VIN it is for as an argument.

So the current change LGTM - it's just that we need to document it as a breaking change and update the usage in homeassistant-myskoda accordingly.

[dvx76]

Or we can set up _notify_callbacks to handle both callback functions with and without arguments, something like this

    def _notify_callbacks(self, vin: Vin) -> None:
        """Execute registered callback functions for the vin."""
        for callback in self._callbacks.get(vin, []):
            try:
                result = callback(vin)
            except TypeError:
                result = callback()

[WebSpider]

@Giermann would you be willing to update your pr?

[WebSpider]

@Giermann would you be willing to update your pr?

[Giermann]

Thanks, @dvx76!
I also included a warning, maybe this way the workaround could be removed later.
Unfortunately now the lint test is failing - of course...

[Giermann]

Still, I am not sure wheter we should really simply provide the VIN to the callbacks.

WebSpider wrote (although he later revised this comment):
"Basically, [...] subscribe_updates [...] only gives you a signal that the vehicle has been updated, but since we do not know what exactly has been updated, we leave that to the callback to handle."

But if the callbacks are interested in the trigger of that change, we could exented _notify_callbacks() to receive and publish that information too. If this is required (in the future?), we should introduce a BaseEvent parameter now instead of a simple VIN, which could be extended later.
Otherwise there would be another change of callback functions...

But I am fine with both - as long as the callback receives any information about the affected VIN.

[WebSpider]

Thanks, @dvx76!
I also included a warning, maybe this way the workaround could be removed later.
Unfortunately now the lint test is failing - of course...

Yes it is, but it is a rather simple one:

myskoda/myskoda.py:856:101: E501 Line too long (125 > 100) 

[Giermann]

I overlooked that, only noticed the now still present "Expected 1 more positional argument (reportCallIssue)", which is by design.

[WebSpider]

I overlooked that, only noticed the now still present "Expected 1 more positional argument (reportCallIssue)", which is by design.

I see, this pyright message will likely disappear when you make the new Vin an optional argument (Vin|None)

Also, when you add pre-commit locally, you can run these tests locally before committing.

[Giermann]

Thanks, but still failing - must be something I don't understand...

[dvx76]

Thanks, but still failing - must be something I don't understand...

/home/runner/work/myskoda/myskoda/myskoda/myskoda.py:859:26 - error: Expected 1 more positional argument (reportCallIssue)

I suspect it's due to the type hint for callback

Callable[[Vin | None], Coroutine[Any, Any, None]

That means "a callable (function) with one argument: either a Vin, or None". Instead what we want to say is "Either a callable with one argument (Vin), or a callable with no arguments.

Untested:

Callable[[Vin | None], Coroutine[Any, Any, None] | Callable[[], Coroutine[Any, Any, None]]

Which isn't very pretty, but correctly expresses what we would now be allowing.

---

But if the callbacks are interested in the trigger of that change, we could exented _notify_callbacks() to receive and publish that information too. If this is required (in the future?), we should introduce a BaseEvent parameter now instead of a simple VIN, which could be extended later.
Otherwise there would be another change of callback functions...

MySkoda.subscribe() is already available to get notifications about individual (MQTT) events. Essentially a passthrough from what the MQTT broker sends out.

The idea of MySkoda.subscribe_updates() is to just notify that vehicle data has been updated, not as a means to report exactly what was updated.

[Giermann]

Sorry for the garbage, but I did not manage to run Lint locally.
Obviously the untested fix from @dvx76 does not work.
Any assistance is greatly appreciated!

[dvx76]

It's just a missing square bracket

Callable[[Vin], Coroutine[Any, Any, None]] | Callable[[], Coroutine[Any, Any, None]]

But the type checker is still upset because it doesn't know what the 'right' signature for the callback function is. I'll work on an alternative. Or maybe we just always include the vin and make it a breaking change. It makes sense to the vin as an argument and the required change in home assistant is minimal.

[dvx76]

Right. It's doable but it's fiddly, especially with the type checker. And in retrospect I don't like the ambiguity of supporting both callback signatures. If it's fine for @WebSpider I'd vote to just add the VIN and mark the change as breaking (we can do that when merging and releasing a new package, don't worry about it here). So this version: https://github.com/skodaconnect/myskoda/pull/502/changes/BASE..bff6966d30e5a869e2f494ff01e401db264fc017

Either that, or make no change at all and handle it fully on the client side, using something like this which bakes the vin into the function passed into myskoda.subscribe_updates:

def make_callback(vin: str):
    async def callback():
        await your_vin_specific_callback_function(vin)
    return callback

for vin in list_of_vins:
    myskoda.subscribe_updates(vin, make_callback(vin))

[WebSpider]

Totally fine by me. Since we're doing a breaking change then, i'll plan some cleanup PRs to remove some deprecated code.

[Giermann]

Maybe it would be better to release this breaking change in two stages.
You could announce the (breaking) change to come in one of the next releases and encourage clients to define the callback routines with an optional parameter first:

async def on_myskoda_update(vin: Vin | None = None):
    latest = myskoda.vehicle(vin | "YOUR_VIN")
    print(f"Latest data for {vin | "YOUR_VIN"}: {latest})

Then, after a few days or weeks, release the new version that provides the VIN to the callback and simplify the example.

[Giermann]

This would require to change homeassistant-myskoda before the change here:

    async def _on_myskoda_update(self, vin: Vin | None = None) -> None:
        if vin is not None and vin != self.vin:
            return

        """Trigger an update of all HA entities when User or Vehicle change.

        Always pass in a copy of the object to force an update.
        """

[WebSpider]

This would require to change homeassistant-myskoda before the change here:

    async def _on_myskoda_update(self, vin: Vin | None = None) -> None:
        if vin is not None and vin != self.vin:
            return

        """Trigger an update of all HA entities when User or Vehicle change.

        Always pass in a copy of the object to force an update.
        """

homeassistant-myskoda is pinned to a specific version of this library. We would have to change this when we update the library version in the integration.

We made it this way so we can release them both independently 👍