[Ovmsdev] Metrics & Notifications: thread safety

[Mark Webb-Johnson]

I don’t think the delay would be an issue of itself, but I do worry about listeners expecting certain behaviour and possible timing implications. For example, vehicle.{h, cpp} uses a MetricModified() listener to pickup on changes to key metrics and fire off notifications (which then lead to further events or actions). What would be the impact of the delivery of the event was delayed from the code path? At the moment, the code expects that MetricModified() will be called before the return from setting the metric value (and hence before processing other CAN bus messages that may affect state).

That said, we faced the same issue with events some time ago, and in that case modified ovms_events(), to introduce a queue and task for handling delivery of events. It wouldn’t be difficult to do something similar for metrics. Or perhaps merge the two (so metric listener notifications are queued to and delivered by the event task). Or could we simply deliver them as events (metric.X)?

Alternatively, Steve and I have talked before about a command task, with all commands executed through that. Perhaps the biggest culprit for large stack sizes is the requirement to issue commands?

Regards, Mark.

> On 24 Jan 2019, at 4:27 AM, Stephen Casner <casner at acm.org> wrote:
> 
> Using a separate task is particularly appealing for the stack
> requirements consideration.  Would the extra delay be a problem for
> any of the recipients?
> 
>                                                        -- Steve
> 
> On Wed, 23 Jan 2019, Michael Balzer wrote:
> 
>> Jakob,
>> 
>> bool, int and float are 32 bit values, so reads and writes are atomic on ESP32. A read-modify-write cycle would not be atomic, but the metrics API does not
>> provide that kind of operation.
>> 
>> The effect of SetModified(false) is not to reset the modified bits, it just updates m_defined & m_stale. So this does not cancel out a concurrent
>> SetModified(true). However, the current implementation does not guarantee NotifyModified() will happen at most once for the same change.
>> 
>> A solution could be to decouple NotifyModified() from SetModified(), possibly moving it to a dedicated task as well. I've been a little bit discomforted since
>> the beginning with metrics listeners being called within the modifying context. Neither the metric modifier knows about the listener time and stack
>> requirements, nor does the listener know about the restrictions of the current context. For example, the transmission of the metric change via network can run
>> within the CAN RX context. Even the creation of a possibly complex notification can be coupled directly to a metrics change.
>> 
>> So maybe decoupling the callback execution is the next step here.
>> 
>> Regards,
>> Michael
>> 
>> 
>> Am 23.01.19 um 16:28 schrieb Jakob Löw:
>>> Hey,
>>> 
>>> OvmsMetricBool, -Int and -Float don't seem to use a mutex, but afaik
>>> there is no garantuee for them to be atomic. A simple fix would be to
>>> make m_value a std::atomic<float> etc. or use a mutex just as with
>>> OvmsMetricString.
>>> Furthermore there might be a small race condition when two tasks modify
>>> a metric. Assuming the following base situation:
>>> 
>>> OvmsMetricString dummy("dummy");
>>> dummy.SetValue("a");
>>> 
>>> // some other code...
>>> 
>>> dummy.SetValue("a");
>>> // now between ovms_metrics.cpp line 793 and 794 the thread is interrupted and this happens:
>>> 
>>> dummy.SetValue("b");
>>> // the value is modified, all change listener are called but none resets the modified flag.
>>> 
>>> // now execution continues with line 794 of dummy.SetValue("a"); from above
>>> // which essentially does SetModified(false);
>>> // but in fact the current value is "b" so it was modified.
>>> 
>>> This also applies to all other metric types, as they all seem to call
>>> SetModified after changing the value.
>>> The easy fix would be to put the SetModified into the mutex lock, but
>>> that would result in a deadlock when one of the change listeners calls
>>> SetValue.
>>> I don't see an better way to avoid that race condition though :(
>>> 
>>> - Jakob
>>> 
>>> On Tue, 2019-01-22 at 15:49 +0100, Michael Balzer wrote:
>>>> https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/comm
>>>> it/8003fc0da759d5d33682c54cbf38840823477fbc
>>>> 
>>>> Brain surgery. A second pair of eyes on this would be good.
>>>> 
>>>> Regards,
>>>> Michael
>>>> 
>>>> _______________________________________________
>>>> OvmsDev mailing list
>>>> OvmsDev at lists.openvehicles.com
>>>> http://lists.openvehicles.com/mailman/listinfo/ovmsdev
>> 
>> --
>> Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
>> Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
>> 
> _______________________________________________
> OvmsDev mailing list
> OvmsDev at lists.openvehicles.com
> http://lists.openvehicles.com/mailman/listinfo/ovmsdev