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

Working subscription #240

Merged
merged 10 commits into from
Nov 15, 2024
Merged

Working subscription #240

merged 10 commits into from
Nov 15, 2024

Conversation

wirew0rm
Copy link
Member

@wirew0rm wirew0rm commented Nov 12, 2024

Draft due to fair-acc/opencmw-cpp#358 not yet being merged and some opinionated changes.

See the individual commit messages for more information on the changes.

The goal of this branch is to allow to reliably subscribe to a service.
To that end it changes the RemoteSource to do proper state management and reconnect in case of subscription errors.

Also this adds more diagnostics with counters, tags, printouts and a small cli tool to analyze the total latency of the processing and networking stack.

The current state will still accumulate some latency for update rates higher than ~5Hz on my system and will need further improvements, but it should be enough to build upon while we try to follow up these issues further.

Copy link

PR produced different images:

chart_0001.png

Got:

chart_0001.png

Expected:

chart_0001.png

Diff:

chart_0001.png

@wirew0rm wirew0rm force-pushed the workingSubscription branch 2 times, most recently from 124a900 to 7e89e9f Compare November 13, 2024 20:42
@wirew0rm wirew0rm marked this pull request as ready for review November 14, 2024 09:50
@wirew0rm wirew0rm marked this pull request as draft November 14, 2024 11:05
@wirew0rm
Copy link
Member Author

Have to change RemoteSource and Scheduler handling to do all emscripten_fetch handling in the worker thread, control the scheduler state via messages and use emscripten_sleep() to process results in the worker threads.

@wirew0rm wirew0rm force-pushed the workingSubscription branch from 7e89e9f to 69f3599 Compare November 15, 2024 12:20
src/ui/App.hpp Outdated Show resolved Hide resolved
src/ui/App.hpp Outdated Show resolved Hide resolved
Copy link
Member

@RalphSteinhagen RalphSteinhagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and saw that it locally fixes the subscription drop/reconnect. Well done. 👍

There are a still a lot of verbose debugging print-outs which should be sanitized as they obfuscate unrelated debugging.

Depending on the local/specific case, I'd recommend either via:

  • removing them entirely. If someone needs it again, they can always re-added later. Leaving them in means someone else is paying the price for reading/cleaning-up.
  • for critical info/warning messages, convert them to, for example:
    • UI: popup info/warning/error messages
    • GR: throw an exception that is intercepted and forwarded by the calling block to the scheduler (and eventually UI) or send an explicit message via the Block's msg port
  • add a block-local circular history buffer that can be displayed/inspected during runtime.

The required changes are minor and do not require further review. Feel free to merge once the changes are in and the CI has passed.

@wirew0rm wirew0rm force-pushed the workingSubscription branch 2 times, most recently from f4955d6 to 6e46352 Compare November 15, 2024 15:16
@wirew0rm wirew0rm marked this pull request as ready for review November 15, 2024 15:25
change sampling rate to 10KS/s and remove unused parameter

Signed-off-by: Alexander Krimm <[email protected]>
@wirew0rm wirew0rm force-pushed the workingSubscription branch from 6e46352 to 022bdff Compare November 15, 2024 17:02
Signed-off-by: Alexander Krimm <[email protected]>
Needs gnuradio4 headers due to tag definitions being used.

Signed-off-by: Alexander Krimm <[email protected]>
This is required for the emscripten fetch calls to be issued in the main
browser thread. If they are issued on another thread, they are only
processed once the thread is finished.

Otherwise we would have to investigate if there is a way to control
on which thread the callbacks should be processed or we would have to
proxy the emscripten_fetch calls to the main thread.
This is further complicated because it needs to be transparent to the
part of the code that also compiles to native.

Set graph threadpool size to one

Signed-off-by: Alexander Krimm <[email protected]>
@wirew0rm wirew0rm force-pushed the workingSubscription branch from 022bdff to dabad99 Compare November 15, 2024 17:07
Moves setting up the subscription to the start and stop methods instead
of performing them on settings change, which lead to duplicate
subscriptions.
Also fixes a bug that would not actually perform any unsubscribes making
the subscriptions live on forever.

Also introduces propagation of Timing Events.

Signed-off-by: Alexander Krimm <[email protected]>
@wirew0rm wirew0rm force-pushed the workingSubscription branch from dabad99 to 2739160 Compare November 15, 2024 17:59
@wirew0rm wirew0rm merged commit bb0c188 into main Nov 15, 2024
7 checks passed
@wirew0rm wirew0rm deleted the workingSubscription branch November 15, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants