goToMain / libosdp

Implementation of IEC 60839-11-5 OSDP (Open Supervised Device Protocol); provides a C library with support for C++, Rust and Python3
https://libosdp.sidcha.dev
Apache License 2.0
130 stars 69 forks source link

PD event queue behavior when queue is full #94

Closed bjyoungblood closed 1 year ago

bjyoungblood commented 1 year ago

Is your feature request related to a problem? Please describe. When a PD's event queue is full, calls to osdp_pd_notify_event return -1 to indicate a failure. In our use case, we would prefer to have the oldest event dropped from the queue instead of blocking newer events.

Describe the solution you'd like In order to maintain backward-compatibility, the queue-full behavior could be controlled by a flag stored on the osdp_pd struct or on the osdp_queue struct. A new public function void osdp_pd_set_queue_mode(osdp_t *ctx, enum osdp_queue_mode mode) would allow this flag to be changed.

Additionally, it would be nice if the library could report when it drops an event via an optional callback function, for example typedef void (*osdp_event_dropped_callback_t)(struct osdp_event *dropped_event) or something similar.

Describe alternatives you've considered I considered making it possible to swap out the queue implementation entirely by adding callback functions for init/enqueue/dequeue to the osdp_pd struct and falling back to the existing pd_event_* functions, but that seemed like overkill to solve this particular issue.

Additional context If this proposal makes sense as-is, I'm happy to implement it and submit a PR. I'm also open to feedback on alternatives.

sidcha commented 1 year ago

In our use case, we would prefer to have the oldest event dropped from the queue instead of blocking newer events.

@bjyoungblood, Why is the CP not pulling these events from the PD? is it because the CP is down (likely) or is it because the PD is producing more data than the CP can consume(unlikely)?

If it is the former, then adding a new method (osdp_pd_flush_events) to flush all queued events [*] for that PD makes more sense than modifying osdp_pd_notify_event to drop events silently. From a pure design perspective, If PD event queue is full, the app is better off knowing about the failure upfront, in more obvious ways, than calling another API to get number events dropped.

[*] Another possible extension to this idea is to drop the top N queued events (instead of all). I don't prefer this as much reasoning: if CP hasn't refreshed the PD in a while, then those events would probably be stale; hence flushing all seems to be a better option. But if you feel that is not the case, we can certainly flush N (where, N=1 would be what you asked for before).

bjyoungblood commented 1 year ago

@bjyoungblood, Why is the CP not pulling these events from the PD? is it because the CP is down (likely) or is it because the PD is producing more data than the CP can consume(unlikely)?

Primarily because the CP is down. For our use case, we generally aren't expecting the CP to be down for a long time (a typical reason would be the CP rebooting after a firmware update).

If it is the former, then adding a new method (osdp_pd_flush_events) to flush all queued events [*] for that PD makes more sense than modifying osdp_pd_notify_event to drop events silently. From a pure design perspective, If PD event queue is full, the app is better off knowing about the failure upfront, in more obvious ways, than calling another API to get number events dropped.

[*] Another possible extension to this idea is to drop the top N queued events (instead of all). I don't prefer this as much reasoning: if CP hasn't refreshed the PD in a while, then those events would probably be stale; hence flushing all seems to be a better option. But if you feel that is not the case, we can certainly flush N (where, N=1 would be what you asked for before).

That makes sense to me, and I agree with your point about stale events. I wouldn't want a CP to come online and start receiving events from 5 minutes ago (or even 1 minute ago).