Attribute observer callbacks are called with heartbeat until disabled - after first called

[eliao]

This seems odd to me, particularly because they are labeled "__on_change".

eg: if I do a
vehicle.add_attribute_observer('mode', mode_callback)

mode_callback is called every heartbeat, regardless of mode change.

[mrpollo]

https://github.com/diydrones/dronekit-python/blob/master/droneapi/module/api.py#L440

        elif typ == "HEARTBEAT":
            self.__on_change('mode', 'armed')

not sure why we did this, @geeksville do you remember this?

[geeksville]

Using heartbeat to detect mode changes is a good thing. But yep, it would be much better to also keep a cache of mode and armed inside of the vehicle object and only publish mode and armed if the corresponding boolean changes state.

[mrpollo]

perfect, I think this is what @eliao is calling odd here, I'll submit a PR with some caching, if the mode is still the same we won't publish __on_change('mode') same with armed

[mhct]

@mrpollo Perhaps the cache should be done for all other properties as well, since there is no check for evaluating if they have changed or not.

[hamishwillee]

@mrpollo @mhct Yes. The bug shows up on all properties not just mode.

[hamishwillee]

@mrpollo FYI, it appears that the callback is called more often than the heartbeat - when testing #184 I was getting "lots" of records a second and the heartbeat only comes in once a second. I suspect this means that it is updated whenever a relevant message containing the attribute comes from the vehicle. That would certainly make more sense than updating on heartbeat. Can you confirm? (Note, I assumed this to be correct so that is what #184 states)

In #184 I explain how you might cache the value and provide notification only when the value changes. I still think this caching should be done by our API.

When do you think we might get some resource on this?

[tcr3dr]

@hamishwillee I'll take over this issue. I think classifying it as a feature (attribute caching) makes it worth a minor release, so it's worth documenting for the 1.3.x branch. I will focus effort on #184 first.

[hamishwillee]

@tcr3dr Just to be precise:

• The way I think this works is that we publish observed values whenever messages come in with those values. The documentation delivered by Improve Rangefinder attributed documentation #184 assumes this. Can you confirm?
• It could be that you are saying instead of the above we already cache the incoming values, and what is reported is the last received value, which is reported at some other schedule. If this is the case I'd say it is a bug

Irrespective of which of the above is happening, arguably we should be publishing the observed value just on change. I think you're saying that we're not going to do that, but stick with the way it is documented in #184.

Can you confirm please the above points (particularly which of the three options presented here we are aiming for)

[tcr3dr]

@hamishwillee Your first point is correct. We do no caching yet, so we are erroneously sending "change" updates when a change may not have occurred.

So, I can prepare a patch to this, but I do not have enough test cases yet to confirm it will work well. In the meantime, I propose we go ahead, document this behavior (as a bug) via #184, and for the release that fixes this issue we update the documentation accordingly.

We will still want the documentation to read "Note: Versions before 1.x.x exhibited a bug where..." for the short term, so having it in the docs now is a good start.

[hamishwillee]

@tcr3dr Thanks very much. Completely agree.

[tcr3dr]

Should be aided by #446. Other attribute callbacks called "too frequently" should be submitted as new bugs so we can make a judgement call on them.