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
139 stars 73 forks source link

PD does not know when it is not talking with CP #133

Closed rm5248 closed 1 year ago

rm5248 commented 1 year ago

Is your feature request related to a problem? Please describe. The PD that we are making has a number of MFGREP responses that it sends out without being prompted by the CP. If the communications go offline, libosdp will continue to queue up these responses, leading to a large number of messages sent back to the CP at once.

This could potentially lead to an issue with credentials as well, since those messages also get queued up, leading to a situation where if a PD is not connected to the CP, you can scan a card and then it will send the credential once communications are restored.

Describe the solution you'd like There are several things that I think need to happen:

Ideas for API:

osdp_pd_last_poll() <-- get a time when the last poll to this device happened.  
osdp_pd_set_max_mesage_timeout() <-- set how long to keep messages to send back to CP before discarding

typedef uint64_t (*osdp_get_time)() <-- function pointer that libosdp calls to get the current time.  If not set, it can just call time(NULL).  

Additional context I can work on implementing this, but I wanted to get some feedback on the API first before I start work on it.

sidcha commented 1 year ago

I don't think it's wise to let libosdp do the flow control of such unsolicited MFGREP responses, after all, they are generated by the PD app. So, IMHO the PD app should look at the current PD status (online/sc-active) and then queue those messages. The PD is never intended to generate events when it's not connected to the CP.

Still, libosdp should not queue so many message as it would, as you point out, lead to misbehaviour. So I think a new API to flush all currently queued events/commands is in order. Will that help you resolve this issue?

rm5248 commented 1 year ago

I don't think it's wise to let libosdp do the flow control of such unsolicited MFGREP responses, after all, they are generated by the PD app. So, IMHO the PD app should look at the current PD status (online/sc-active) and then queue those messages. The PD is never intended to generate events when it's not connected to the CP.

That is reasonable to me, assuming the PD can get that online status. Potentially osdp_pd_notify_event could fail if a poll has not been received in a certain amount of time.

Still, libosdp should not queue so many message as it would, as you point out, lead to misbehaviour. So I think a new API to flush all currently queued events/commands is in order. Will that help you resolve this issue?

There is already a function that should let us do that(osdp_pd_flush_events), the problem is that we don't have a way to detect when we should call it.

The other alternative would be to also pass OSDP_POLL events back up with the command callback, that way the calling code can keep track of if the poll events are still coming in or not. It is a bit cleaner to keep track of that in libosdp IMO.

sidcha commented 1 year ago

assuming the PD can get that online status

This can be done by using osdp_get_sc_status_mask and osdp_get_status_mask methods.

rm5248 commented 1 year ago

Isn't that only on the control panel side, so that the control panel can see what PDs are online?

sidcha commented 1 year ago

No, it's a common API for CP and PD. In case of PD, only bit-0 has valid data.

rm5248 commented 1 year ago

I have just tested that out, and it does not work on the PD side. Here is some simplified code:

  ctx = osdp_pd_setup(&info_pd);
  uint8_t status_mask = 0;
  int x = 0;
  while(1) {
    osdp_pd_refresh(ctx);
    osdp_get_status_mask(ctx, &status_mask);
    if((x++ % 20) == 0)
      printf("status_mask %d\n", status_mask);

     // sleep for a bit here...
  }

The status_mask that is printed out is always 1, no matter if the PD is currently being polled by the CP or if it has not received a message in several seconds.

rm5248 commented 1 year ago

Upon further investigation, the osdp_get_sc_status_mask does work for getting the secure channel status(it will timeout if no communications are received in a certain period of time). This does not help in the case where communications are plain text(no SCBK).

sidcha commented 1 year ago

Yes, this is expected. The PD has no notion on state except when it comes to secure channel. Otherwise all other commands can be sent by the CP at any time.

But, we could introduce the "online" state based on last poll time like we do for the secure channel "active" state.

rm5248 commented 1 year ago

But, we could introduce the "online" state based on last poll time like we do for the secure channel "active" state.

Yes, that is what I am looking for.

I can create something based off of the current code for checking the secure channel, that shouldn't be too hard. What should the API look like? int osdp_pd_is_online(osdp_ctx*)? Or perhaps osdp_pd_has_cp_communication(osdp_ctx*)?

sidcha commented 1 year ago

No, we could just make osdp_get_status_mask honest about its reporting, to avoid having to add a new API.

sidcha commented 1 year ago

@rm5248 Please take a look at d934636 and let me know if this works for you (I haven't tested it).

rm5248 commented 1 year ago

That won't work as-is, because the pd variable is not initialized.

As far as I can tell, that also won't work because the pd->tstamp is never updated when a PD receives data.

sidcha commented 1 year ago

That won't work as-is, because the pd variable is not initialized.

Right. I didn't even compile that change. :)

As far as I can tell, that also won't work because the pd->tstamp is never updated when a PD receives data.

It is updated. See origin/osdp_pd_status. I added tests to make sure it works.