<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Jakob,<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
So maybe decoupling the callback execution is the next step here.<br>
<br>
Regards,<br>
Michael<br>
<br>
<br>
<div class="moz-cite-prefix">Am 23.01.19 um 16:28 schrieb Jakob Löw:<br>
</div>
<blockquote type="cite" cite="mid:1548257309.4139.1.camel@m4gnus.de">
<pre class="moz-quote-pre" wrap="">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.</pre>
</blockquote>
<blockquote type="cite" cite="mid:1548257309.4139.1.camel@m4gnus.de">
<pre class="moz-quote-pre" wrap="">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:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""><a class="moz-txt-link-freetext" href="https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/comm">https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/comm</a>
it/8003fc0da759d5d33682c54cbf38840823477fbc
Brain surgery. A second pair of eyes on this would be good.
Regards,
Michael
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
OvmsDev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:OvmsDev@lists.openvehicles.com">OvmsDev@lists.openvehicles.com</a>
<a class="moz-txt-link-freetext" href="http://lists.openvehicles.com/mailman/listinfo/ovmsdev">http://lists.openvehicles.com/mailman/listinfo/ovmsdev</a>
</pre>
</blockquote>
</blockquote>
<br>
<pre class="moz-signature" cols="160">--
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26
</pre>
</body>
</html>