Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ReadHandler] Review SyncReportScheduler Synching logic #28083

Open
lpbeliveau-silabs opened this issue Jul 19, 2023 · 1 comment
Open

[ReadHandler] Review SyncReportScheduler Synching logic #28083

lpbeliveau-silabs opened this issue Jul 19, 2023 · 1 comment
Labels
icd Intermittently Connected Devices

Comments

@lpbeliveau-silabs
Copy link
Contributor

lpbeliveau-silabs commented Jul 19, 2023

It seems more exhaustive testing is also require on the sync mechanism to ensure it functions as expected.

The targeted behavior needs to be well defined before hand and we need to reach a compromise between responsiveness and minimizing wake-up numbers for ICDs.

@lpbeliveau-silabs
Copy link
Contributor Author

Several corner case need to be tested when behavior is defined such as:
Handler becoming dirty during an Engine::Run(), currently:

handlerx becomes dirty mid run, OnBecameReportable will be called, then CalculateNextReport will either
give a timeout a at handlerx’s min timestamp (or the common min if another handler is also reportable at its min)
or
give a timeout of 0, which will result in the ReportTimerCallback to be called at 0.
The timer expiration will be processed after the run is complete
Therefore, during the run, when OnSubscriptionAction is emitted as handlers emit their reports, CalculateNextReport will see handlerx as reportable now and thus try to sync currently reportable handlers with it, which they will if they have a min of 0.

Current desired behavior can be summarized by:

  1. Each handler has timestamps for "must not report before" and "must report by". These timestamps need to be recomputed every time the handler sends SubscribeResponse or sends the last chunk of a non-priming report. This part I believe we have and this works correctly.
  2. We don't really care about random runs happening whenever; they can just happen.
  3. We want to make sure we are not doing separate wake-ups for multiple things hitting their min-interval at slightly different times. To handle this, we want to have any run scheduling we do to handle min-interval expiration coalesce things to some extend (issue filed to determine extent).
  4. We want to make sure we are not doing separate wake-ups for multiple things hitting their max-interval at slightly different times. To handle this, we are considering things as "reportable now" even if not dirty and the max-interval has not been reached, in some cases. The "sync timestamp" mechanism is supposed to implement this.

The key issue with what we are doing now:

We are not "syncing" the reporting in any meaningful way. When a "run" (in the reporting engine sense) happens, reporting can be spread out over 10s of seconds, with multiple runs as we work our way through all the reportable things. What we are really doing is marking some handlers as reportable earlier than they would have been otherwise

One possible path to a solution would be to design as suggested here:

We should schedule max-interval for "last regularly scheduled wake-up that is smaller than the min max-interval" and mark for reporting at that point "all handlers that have max intervals between that wake-up and the next wake-up" or something along those lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
icd Intermittently Connected Devices
Projects
None yet
Development

No branches or pull requests

1 participant