[Ovmsdev] Fwd: [openvehicles/Open-Vehicle-Monitoring-System-3] f84837: Server V2 & V3: fix registration for notifications...

[Mark Webb-Johnson]

Michael,

Thanks for the clarification. That makes more sense.

I do find the use of a static outside the class kind of kludgy/ugly. What if RegisterReader firstly iterated over the map to see if there was a free slot below m_nextreader? Or change the map to an array? Then we could simply keep the reader ID as a member variable of the class.

Worth it, or not?

Regards, Mark.

> On 18 Nov 2019, at 5:35 PM, Michael Balzer <dexter at expeedo.de> wrote:
> 
> Mark,
> 
> Am 18.11.19 um 02:37 schrieb Mark Webb-Johnson:
>> I don’t think this fix is correct. Seeking clarification of the thinking behind it.
>> 
>> Currently, MyOvmsServerV2Reader is initialised to 0. Then, OvmsServerV2::OvmsServerV2 creates the reader (and sets MyOvmsServerV2Reader to it) if it is zero. Then OvmsServerV2::~OvmsServerV2 deregisters (clears) the reader, but does not set MyOvmsServerV2Reader to 0.
>> 
>> The change is to OvmsServerV2::OvmsServerV2 to register the reader (without setting MyOvmsServerV2Reader) if MyOvmsServerV2Reader is 0.
>> 
>> This seems to leak one reader every time the OvmsServerV2 is stopped/started, as without storing in MyOvmsServerV2Reader, the old reader will never be cleared upon deregistering. Or am I missing something?
> 
> You miss the MyOvmsServerV2Reader being given to the second registration call to be reused.
> 
> The current implementation of reader registration is allocating one of 32 available bits for a new reader. The bit allocation is permanent, the servers accordingly only allocate their ids once.
> 
> But the reader entry in the list of active notification readers isn't permanent. To be able to re-register for a previously allocated bit number, I introduced the second RegisterReader() method some time ago. That was for the webserver context -- registering a new id on every new websocket connection would run out of ids very soon.
> 
> It became apparent (from a user report on the weekend) that both v2 & v3 servers did not register as readers on a second start, so have the same issue.
> 
> You can test the previous implementation and the fix by watching "notify status".
> 
>> It seems that the better solution is to change OvmsServerV2::~OvmsServerV2 to set MyOvmsServerV2Reader=0 after deregistering (which is the original bug, imho)?
> 
> That would actually leak reader bits due to the permanent allocation.
> 
> It would have been an option to add a bit mask for the use/free status so you don't need to permanently store and reuse the allocated reader id. IIRC I decided against that because the v2&v3 servers had that scheme of permanent storage. Thinking about it now, that would be a more clean, more standard way option for the notify API, maybe we should do that.
> 
> Regards,
> Michael
> 
> 
>> Regards, Mark.
>> 
>>> Begin forwarded message:
>>> 
>>> From: Michael Balzer <noreply at github.com <mailto:noreply at github.com>>
>>> Subject: [openvehicles/Open-Vehicle-Monitoring-System-3] f84837: Server V2 & V3: fix registration for notifications...
>>> Date: 17 November 2019 at 11:22:46 PM HKT
>>> To: mark at webb-johnson.net <mailto:mark at webb-johnson.net>
>>> 
>>>  Branch: refs/heads/master
>>>  Home:   https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3 <https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3>
>>>  Commit: f84837a2ce40d32769134d0bb5fe6dee09d842b3
>>>      https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/commit/f84837a2ce40d32769134d0bb5fe6dee09d842b3 <https://github.com/openvehicles/Open-Vehicle-Monitoring-System-3/commit/f84837a2ce40d32769134d0bb5fe6dee09d842b3>
>>>  Author: Michael Balzer <balzer at expeedo.de <mailto:balzer at expeedo.de>>
>>>  Date:   2019-11-17 (Sun, 17 Nov 2019)
>>> 
>>>  Changed paths:
>>>    M vehicle/OVMS.V3/components/ovms_server_v2/src/ovms_server_v2.cpp
>>>    M vehicle/OVMS.V3/components/ovms_server_v3/src/ovms_server_v3.cpp
>>> 
>>>  Log Message:
>>>  -----------
>>>  Server V2 & V3: fix registration for notifications on restart
>>> 
>>> 
>> 
>> 
>> 
>> _______________________________________________
>> OvmsDev mailing list
>> OvmsDev at lists.openvehicles.com <mailto:OvmsDev at lists.openvehicles.com>
>> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvehicles.com/pipermail/ovmsdev/attachments/20191119/3a182dde/attachment.htm>