maximkulkin / esp-homekit

Apple HomeKit accessory server library for ESP-OPEN-RTOS
MIT License
1.12k stars 170 forks source link

Request for Information: Once a Value is Updated, How to Notify Clients? #161

Closed mriksman closed 4 years ago

mriksman commented 4 years ago

Hey,

If I need to update a value like;

active->value = HOMEKIT_UINT8(1);

Will it automatically notify connect client apps? So if I had the iOS Home App open, it will automatically change immediately?

Or do I need to call

homekit_characteristic_notify(active, active->value);

Looking at accessories.c it appears that all this does is fire off the callback function.

So - how does the Home app get immediately notified of a change?

My issue is, I am connecting to a Mitsubishi Air Conditioner. I read back the currently set TARGET_HEATING_COOLING_STATE (Heat, Cool, Auto), which can be changed by a remote controller, and I need to update the value in HomeKit. I want this to appear immediately in the Home app, but I don't want it to fire the callback function. The callback function should only be called when I change the mode from the Home app (so I can then send this command to the Mitsubishi Air Conditioner).

mriksman commented 4 years ago

Ahhh #157 answers it.

So, this is causing me an issue...

If Home app changes the TARGET_HEATING_COOLING_STATE, I need a function to fire off so I can send the settings to the Mitsubishi controller.

But if I read back a state from the Mitsubishi controller (Off, Heat, Cool, Auto), how can I update TARGET_HEATING_COOLING_STATE without firing off the callback function?

As it stands now, I could get in a position where;

  1. Home app change triggers callback function
  2. Callback function queues the command to send via UART to the Mitsubishi controller.
  3. Before the queue executes and updates the settings, a poll on the Mitsubishi controller notices that the HomeKit value is not the same as the value it has just read (because we changed the value in 1).
  4. Code changes the value and calls the notify function, causing the callback function to fire, and send another command to the queue - overriding the previous command.

Any thoughts?

maximkulkin commented 4 years ago

Well, first, you can make your updates function idempotent, so if you update everything correctly, the second command to your Mitsubishi controller will be the same as the first one, so, a bit inefficient, but it should do the work.

But in general, here is a hint on how to deal with those situations: setters. Setters are usually called when values are updated from HomeKit, you do not use setters to set values from inside your code (at least, you shouldn't). Then you can have a setter that handles updates from HomeKit and thus you do not need callback on your characteristic, thus no double update when you will call homekit_characteristic_notify().

mriksman commented 4 years ago

Ahh. I'll take a look at .setter and .getter.

So to update Home app immediately, you must call homekit_characteristic_notify(). You may or may not have a .callback function attached to this.

You can use .setter to fire off a function when the Home app updates the value. But I'm noticing that the value isn't updating?

void update_settings_callback() {
  homekit_accessory_t *accessory            = accessories[0];
  homekit_service_t *thermostat_service     = homekit_service_by_type(accessory, HOMEKIT_SERVICE_THERMOSTAT);
  homekit_characteristic_t *target_state    = homekit_service_characteristic_by_type(thermostat_service, HOMEKIT_CHARACTERISTIC_TARGET_HEATING_COOLING_STATE);
  ESP_LOGW(TAG, "target state %d", target_state->value.int_value );

Seems if you use .setter then it doesn't automatically update the internal characteristic value...?

How is .getter used?

mriksman commented 4 years ago

I also noticed .setter_ex which gives me access to _ch characteristic.

mriksman commented 4 years ago

@maximkulkin I've got a mix of .setter_ex and if checks to prevent sending an update packet when it isn't required.

void update_settings_callback(homekit_characteristic_t *_ch, homekit_value_t value) {
    _ch->value = value;
    homekit_characteristic_notify(_ch, value);

    // notify the polling task that there is a change of settings
    xTaskNotifyGive(mitsubishi_poll_task_handle);
}

I've noticed when I open up the Thermostat in the Home app, it will trigger an 'Update Characteristic' for TARGET_HEATING_COOLING_STATE, hence the additional requirements to perform if checks.

>>> HomeKit: [Client 54] Update Characteristics
W (523626) main: update settings
W (523626) main: Target Heating Cooling State
W (523626) main: Target Heating Cooling State

Code is over at https://github.com/mriksman/esp32_homekit_mitsubishi if you're interested in providing any tips/thoughts.

maximkulkin commented 4 years ago

If you use a setter, you do not need a callback. Callback was added as alternative to getter/setter in case you want to store everything inside characteristic.value and you don't want to bother setting it yourself. However, in case of notifications from both your code and HomeKit, notifications are not that useful.

Comments about the code:

  1. Since you already have characteristic pointers in update function, there is no need to compare current characteristic type, just compare _ch with characteristic pointers. Although, if you want different behavior on different characteristics change, why not to create a different functions and assign them as callbacks, instead of having one function and checking which characteristic triggered callback in runtime?!
  2. Why do you create all accessory database in runtime ? It would be much easier to create everything statically, so you will have all pointers to all characteristics and will not have to search for them?
mriksman commented 4 years ago

Thanks for your comments.

I need .callback so I can synchronise ACTIVE and TARGET_HEATING_COOLING_STATE.

  1. That's a great point - I didn't think of using pointers for comparison. Will also separate the functions.
  2. It is just the 'template' I'm using for my code - based on the dynamic TV/Animation code I created.
maximkulkin commented 4 years ago

I need .callback so I can synchronise ACTIVE and TARGET_HEATING_COOLING_STATE.

My point is: the way you want it is not gonna work. You can do same stuff right inside setter, there is nothing special about callbacks. Because of your template you're forced to write MORE code. My suggestion: use a different template.

mriksman commented 4 years ago

But when I read back the TARGET_HEATING_COOLING_STATE in the serial read task from the Mitsubishi AC controller over serial, I also need to update ACTIVE. And when I set TARGET_HEATING_COOLING_STATE in the Home app, I also need to update ACTIVE.

So if I use a setter, then I can only do the latter? Or I can copy the same code in setter, and use it in my serial read task - but now I've duplicated code.

Doesn't using a callback cover both these scenarios in one location?

maximkulkin commented 4 years ago

But when I read back the TARGET_HEATING_COOLING_STATE in the serial read task from the Mitsubishi AC controller over serial, I also need to update ACTIVE. And when I set TARGET_HEATING_COOLING_STATE in the Home app, I also need to update ACTIVE.

Let me tell you about functions: it's like giving a name to a piece of code. You can create a function and put code in it. Then you can call that function from other code and the code inside function will be executed. And you can call a function from multiple places too.

We've started from the question of how to avoid multiple callback executions. Is it already solved?

mriksman commented 4 years ago

Fair point. Thanks for the feedback.