A solution for this could be to let SignalEvent() create a copy of the object pointed to, and let the event task free that after calling all listeners. That wouldn't work for nested objects, but would be sufficient for standard event data transport. Am 07.04.2018 um 11:51 schrieb Michael Balzer:

Mark,

event listeners currently get pointers to objects in the user data field. If we change to asynchronous execution like this, those pointers are no longer valid if they don't point to static objects.

I'm using event data pointers for the EMCY detail data transport in my SEVCON module so would need to change that.

I haven't checked if / where we currently use event data pointers in the framework.

Regards, Michael

Am 07.04.2018 um 11:26 schrieb Mark Webb-Johnson:

...

I’m seeing a timing/stack/mutex/whatever issue with the new 3.1 modules, regarding sd card auto-mounting at boot.

Currently we are using a freertos timer to auto-mount this 500ms after insertion. That fires off some events (sd.inserted, sd.mounted) and I have a script on sd.mounted that does some things like turn on log file to /sd/ovms.log. That is failing (locking up SD). In general, as Michael pointed out, doing big things in freertos timers is not recommended.

I tried a quick fix to change to use ticker.1 and housekeeping timers. But, then in ticker.1 we SignalEvent sd.inserted and sd.mounted, and that leads to a whole new set of problems (raising a signal inside a signal handler).

Overall, it seems messy. The housekeeping task (where we like to run these things) is also very basic at the moment.

I think the better solution maybe to remove the housekeeping task completely, and replace it with an event task reading events off a queue and firing them to all listeners. Then change SignalEvent to queue an event to the event task so that the event task fires the actual event in it’s own time and in it’s own large stack.

The slight change here is that we would still guarantee in-order delivery of events raised, but the caller of SignalEvent would immediately get back control (even if the signal had not been dispatched to all listeners yet).

OK? Any better suggestions?

Regards, Mark

_______________________________________________ OvmsDev mailing list OvmsDev@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

Am 07.04.2018 um 12:50 schrieb Mark Webb-Johnson:

Not sure about freeing the data. It is a void. For a normal malloc pointer I guess it would work, but not for a C++ object. I am happy to provide a SignalEventFree variant to free the void* after the signal is raised. We could add a virtual "EventData" super class, but then would need to encapsulate all simple structs in that, i.e. all system events.

But I think it would also be OK not being able to pass C++ objects but only plain memory structs. Like with FreeRTOS queues, you tell the system how large your object is and everything else is up to you.

I’ve been looking at the way tiT does it (inside lwip). Quite a neat way of packaging up a function call to be executed inside another task. The structure being passed on the queue also contains a semaphore that the sender can wait on, and the semaphore is triggered once the function is done.

How about SignalEventSync and SignalEventAsync variants, with the first one waiting for the signal to be sent before returning?

But the issue is SignalEventSync would not be allowed to be called from within EventTask context (so you can’t call SignalEventSync from within a signal handler). We can protect this within the SignalEventSync function itself by checking the current running task structures.

That sounds complicated. Event handlers raising events would need to be aware of their execution context and need to adapt to it. I think if we make events asynchronous, they should always be (except when raised from within the EventTask, which can be handled by SignalEvent() itself). Another idea that can solve both issues: SignalEvent(event, data, [DoneCallback]) DoneCallback(event, data) would be called by the EventTask after listener execution has finished. The callback can implement memory cleanup and/or synchronous processing (i.e. by a semaphore or task signal) as needed. Regards, Michael

Does that satisfy the requirements?

Regards, Mark.

...

On 7 Apr 2018, at 5:55 PM, Michael Balzer <dexter@expeedo.de> wrote:

A solution for this could be to let SignalEvent() create a copy of the object pointed to, and let the event task free that after calling all listeners.

That wouldn't work for nested objects, but would be sufficient for standard event data transport.

Am 07.04.2018 um 11:51 schrieb Michael Balzer:

...

Mark,

event listeners currently get pointers to objects in the user data field. If we change to asynchronous execution like this, those pointers are no longer valid if they don't point to static objects.

I'm using event data pointers for the EMCY detail data transport in my SEVCON module so would need to change that.

I haven't checked if / where we currently use event data pointers in the framework.

Regards, Michael

Am 07.04.2018 um 11:26 schrieb Mark Webb-Johnson:

...

I’m seeing a timing/stack/mutex/whatever issue with the new 3.1 modules, regarding sd card auto-mounting at boot.

Currently we are using a freertos timer to auto-mount this 500ms after insertion. That fires off some events (sd.inserted, sd.mounted) and I have a script on sd.mounted that does some things like turn on log file to /sd/ovms.log. That is failing (locking up SD). In general, as Michael pointed out, doing big things in freertos timers is not recommended.

I tried a quick fix to change to use ticker.1 and housekeeping timers. But, then in ticker.1 we SignalEvent sd.inserted and sd.mounted, and that leads to a whole new set of problems (raising a signal inside a signal handler).

Overall, it seems messy. The housekeeping task (where we like to run these things) is also very basic at the moment.

I think the better solution maybe to remove the housekeeping task completely, and replace it with an event task reading events off a queue and firing them to all listeners. Then change SignalEvent to queue an event to the event task so that the event task fires the actual event in it’s own time and in it’s own large stack.

The slight change here is that we would still guarantee in-order delivery of events raised, but the caller of SignalEvent would immediately get back control (even if the signal had not been dispatched to all listeners yet).

OK? Any better suggestions?

Regards, Mark

_______________________________________________ OvmsDev mailing list OvmsDev@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

---

OvmsDev mailing list OvmsDev@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

I’ve implemented this (along the lines of what we talked about). I think the free could be done in the way you suggest, or alternatively I have provided an EventStdFree that you can just tack on the end of the SignalEvent call. It just frees the data. I’ve also provided a variant of SignalEvent that can be provided with a size_t for the data. That variant clones the data, and defines a cleanup function to free it. Implementation seems ok. Stack usage looks fine: OVMS# module tasks Number of Tasks = 17 Stack: Now Max Total Heap 32-bit SPIRAM 3FFAFB48 1 Blk esp_timer 388 580 4096 38080 644 21756 3FFBDD5C 2 Blk eventTask 440 1768 4608 0 0 0 3FFBFB58 3 Blk OVMS Events 436 3780 6144 86916 0 6800 3FFC4794 4 Blk OVMS CanRx 432 432 1024 0 0 0 3FFC884C 5 Blk ipc0 392 440 1024 7776 0 0 3FFC8E4C 6 Blk ipc1 392 440 1024 12 0 0 3FFCAC74 9 Rdy IDLE 368 496 1024 0 0 0 3FFCB208 10 Rdy IDLE 356 484 1024 0 0 0 3FFCBF9C 11 Blk Tmr Svc 392 680 3072 0 0 0 3FFCF9F4 16 Blk tiT 492 2348 3072 740 0 5868 3FFD5CA4 17 Blk OVMS SIMCOM 464 2496 4096 4400 0 0 3FFD7C54 18 Blk wifi 432 2304 4096 304 0 1708 3FFDA3EC 19 Blk pmT 408 1672 2560 0 0 0 3FFDAA04 20 Blk OVMS Vehicle 452 452 3072 0 0 0 3FFDEC24 21 Rdy OVMS Console 760 2600 6144 20 15488 12000 3FFDF604 22 Blk mdns 412 1732 4096 104 0 4 3FFE3118 23 Blk OVMS NetMan 728 2392 7168 1528 0 68 It solves my problem with scripts on SD CARD mount for v3.1 hardware. I’ve committed it as is, and fixed most uses of SignalEvent I could find. There are some in the canopen and RT code that I’ve left for @michael. I don’t think notifications will work correctly now because they seem to rely on the SignalEvent returning after all the handlers have processed the event (no longer the case). I’m guessing that needs to be changed to use the DoneFN as a callback. Can you guys have a review of it? I must prefer to run this stuff in one task (Event), but this is a large change and could break a bunch of stuff. Regards, Mark.

On 8 Apr 2018, at 3:27 PM, Michael Balzer <dexter@expeedo.de> wrote:

Am 07.04.2018 um 15:23 schrieb Michael Balzer:

...

Another idea that can solve both issues: SignalEvent(event, data, [DoneCallback])

DoneCallback(event, data) would be called by the EventTask after listener execution has finished. The callback can implement memory cleanup and/or synchronous processing (i.e. by a semaphore or task signal) as needed.

C++ now also has a nice lambda construct with very simple capturing of arbitrary scope parts by copy or reference. That enables nice inline callback definitions like this:

char *data = strdup(message); MyEvents.SignalEvent("simple.event", (void*)data, [data](){ free(data); });

…or…

SomeObject *clone = new SomeObject(original); MyEvents.SignalEvent("object.event", (void*)clone, [clone](){ delete clone; });

Regards, Michael

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev

do we actualy need an additional signal on notifications? What is the use case?

Purely to allow scripts to be triggered and do something. Regards, Mark.

On 9 Apr 2018, at 2:10 AM, Michael Balzer <dexter@expeedo.de> wrote:

Looks good. I've changed the CANopen and RT signals.

Notifications still work, their real transport does not depend on the event but is done by the NotifyReaders() callback queue. But sending the queue entry with the signal can become a race condition now if some future event listener uses that pointer.

A clean solution would be to add a reader key for event listeners that is reset by the callback -- and we also need to add some mutexes… do we actualy need an additional signal on notifications? What is the use case?

Regards, Michael

Am 08.04.2018 um 10:54 schrieb Mark Webb-Johnson:

...

I’ve implemented this (along the lines of what we talked about).

I think the free could be done in the way you suggest, or alternatively I have provided an EventStdFree that you can just tack on the end of the SignalEvent call. It just frees the data.

I’ve also provided a variant of SignalEvent that can be provided with a size_t for the data. That variant clones the data, and defines a cleanup function to free it.

Implementation seems ok. Stack usage looks fine:

OVMS# module tasks Number of Tasks = 17 Stack: Now Max Total Heap 32-bit SPIRAM 3FFAFB48 1 Blk esp_timer 388 580 4096 38080 644 21756 3FFBDD5C 2 Blk eventTask 440 1768 4608 0 0 0 3FFBFB58 3 Blk OVMS Events 436 3780 6144 86916 0 6800 3FFC4794 4 Blk OVMS CanRx 432 432 1024 0 0 0 3FFC884C 5 Blk ipc0 392 440 1024 7776 0 0 3FFC8E4C 6 Blk ipc1 392 440 1024 12 0 0 3FFCAC74 9 Rdy IDLE 368 496 1024 0 0 0 3FFCB208 10 Rdy IDLE 356 484 1024 0 0 0 3FFCBF9C 11 Blk Tmr Svc 392 680 3072 0 0 0 3FFCF9F4 16 Blk tiT 492 2348 3072 740 0 5868 3FFD5CA4 17 Blk OVMS SIMCOM 464 2496 4096 4400 0 0 3FFD7C54 18 Blk wifi 432 2304 4096 304 0 1708 3FFDA3EC 19 Blk pmT 408 1672 2560 0 0 0 3FFDAA04 20 Blk OVMS Vehicle 452 452 3072 0 0 0 3FFDEC24 21 Rdy OVMS Console 760 2600 6144 20 15488 12000 3FFDF604 22 Blk mdns 412 1732 4096 104 0 4 3FFE3118 23 Blk OVMS NetMan 728 2392 7168 1528 0 68

It solves my problem with scripts on SD CARD mount for v3.1 hardware.

I’ve committed it as is, and fixed most uses of SignalEvent I could find. There are some in the canopen and RT code that I’ve left for @michael.

I don’t think notifications will work correctly now because they seem to rely on the SignalEvent returning after all the handlers have processed the event (no longer the case). I’m guessing that needs to be changed to use the DoneFN as a callback.

Can you guys have a review of it? I must prefer to run this stuff in one task (Event), but this is a large change and could break a bunch of stuff.

Regards, Mark.

...

On 8 Apr 2018, at 3:27 PM, Michael Balzer <dexter@expeedo.de <mailto:dexter@expeedo.de>> wrote:

Am 07.04.2018 um 15:23 schrieb Michael Balzer:

...

Another idea that can solve both issues: SignalEvent(event, data, [DoneCallback])

DoneCallback(event, data) would be called by the EventTask after listener execution has finished. The callback can implement memory cleanup and/or synchronous processing (i.e. by a semaphore or task signal) as needed.

C++ now also has a nice lambda construct with very simple capturing of arbitrary scope parts by copy or reference. That enables nice inline callback definitions like this:

char *data = strdup(message); MyEvents.SignalEvent("simple.event", (void*)data, [data](){ free(data); });

…or…

SomeObject *clone = new SomeObject(original); MyEvents.SignalEvent("object.event", (void*)clone, [clone](){ delete clone; });

Regards, Michael

-- Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal Fon 02333 / 833 5735 * Handy 0176 / 206 989 26

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@lists.openvehicles.com> http://lists.openvehicles.com/mailman/listinfo/ovmsdev <http://lists.openvehicles.com/mailman/listinfo/ovmsdev>

_______________________________________________ OvmsDev mailing list OvmsDev@lists.openvehicles.com <mailto:OvmsDev@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@lists.openvehicles.com http://lists.openvehicles.com/mailman/listinfo/ovmsdev