Add continuous mode support to Sipeed SLogic Analyzer#5
Add continuous mode support to Sipeed SLogic Analyzer#5Shoaibashk wants to merge 1 commit intosipeed:slogic-devfrom
Conversation
- Updated device options to include continuous mode configuration. - Introduced a new boolean field `continuous_mode` in the device context structure. - Modified the scan function to enable continuous mode when applicable. - Enhanced the configuration getters and setters to handle continuous mode. - Adjusted the acquisition start function to handle continuous capture, allowing for unlimited sample bytes.
There was a problem hiding this comment.
Pull request overview
This PR adds user-configurable continuous capture support to the Sipeed SLogic Analyzer libsigrok driver by introducing a continuous-mode flag in the device context, exposing it via the config API, and altering acquisition startup behavior to allow unbounded captures.
Changes:
- Add
continuous_modeto the SLogic device context and expose it viaSR_CONF_CONTINUOUSGET/SET. - Modify acquisition startup to treat continuous mode as “unlimited capture”.
- Add/update repository documentation and include local build output logs.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/hardware/sipeed-slogic-analyzer/protocol.h |
Adds continuous_mode field to the device context. |
src/hardware/sipeed-slogic-analyzer/protocol.c |
Implements continuous-mode behavior in acquisition start (unlimited byte target). |
src/hardware/sipeed-slogic-analyzer/api.c |
Exposes continuous mode via config options; initializes continuous_mode during scan. |
README |
Reformats README and adds Windows/MSYS2 build instructions (not mentioned in PR description). |
make-cxx.log |
Adds a large build log artifact. |
make-cxx.exit |
Adds a build exit-code artifact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| devc->cur_samplechannel, devc->cur_samplerate / SR_MHZ(1), | ||
| 1000 * devc->cur_limit_samples / devc->cur_samplerate); | ||
| if (devc->continuous_mode) { | ||
| devc->samples_need_nbytes = UINT64_MAX; |
There was a problem hiding this comment.
Using UINT64_MAX as a sentinel for “unlimited” (samples_need_nbytes) is unsafe here because the value is later used in arithmetic (add/subtract/percent) and comparisons that can overflow or behave incorrectly during long runs (e.g., samples_got_nbytes can wrap, affecting resubmission/stop logic). Prefer an explicit “continuous” code path (skip limit-based arithmetic and always resubmit transfers), or use a dedicated sentinel value with guarded calculations.
| devc->samples_need_nbytes = UINT64_MAX; | |
| /* Use a very large but non-maximal value to avoid overflow in later arithmetic. */ | |
| devc->samples_need_nbytes = UINT64_MAX / 2; |
| sr_info("Need continuous capture at %uch@%uMHz.", | ||
| devc->cur_samplechannel, devc->cur_samplerate / SR_MHZ(1)); | ||
| } else { | ||
| devc->samples_need_nbytes = | ||
| devc->cur_limit_samples * devc->cur_samplechannel / 8; | ||
| sr_info("Need %ux %uch@%uMHz in %ums.", devc->cur_limit_samples, | ||
| devc->cur_samplechannel, devc->cur_samplerate / SR_MHZ(1), | ||
| 1000 * devc->cur_limit_samples / devc->cur_samplerate); |
There was a problem hiding this comment.
The new/modified sr_info() format strings use %u for values that are uint64_t (e.g., cur_limit_samples, cur_samplerate / SR_MHZ(1), and the computed duration). This triggers compiler warnings and can print incorrect values on some platforms. Use the appropriate PRIu64-style macros (or cast to the matching type and use the correct specifier) for these fields.
| @@ -0,0 +1 @@ | |||
| 0 No newline at end of file | |||
There was a problem hiding this comment.
make-cxx.exit looks like a generated build artifact (exit code file). It’s usually better not to commit this kind of build output; consider removing it from the PR and ignoring it via .gitignore if it’s produced locally.
| 0 | |
| This file is intentionally tracked and does not store a generated exit code; it should not be treated as a build artifact. |
| ------------------------------------------------------------------------------- | ||
| README | ||
| ------------------------------------------------------------------------------- | ||
| --- | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
The README changes (Markdown-style headings, horizontal rules at the top, and a new Windows build section) are unrelated to the PR description about SLogic continuous mode support. Please either move these documentation edits to a separate PR, or update the PR description to explicitly include them and ensure the README formatting is intentional/consistent (e.g., consider README.md if switching to Markdown).
| devc->cur_samplechannel = | ||
| devc->limit_samplechannel; | ||
| devc->cur_samplerate = devc->limit_samplerate; | ||
| devc->continuous_mode = TRUE; |
There was a problem hiding this comment.
scan() initializes devc->continuous_mode = TRUE, which makes captures unbounded by default and effectively ignores SR_CONF_LIMIT_SAMPLES unless the client explicitly disables continuous mode. This is a behavior change that can lead to sessions that never end and unbounded memory growth. Consider defaulting continuous_mode to FALSE (and setting a sane default for cur_limit_samples), or only enabling it when the user sets SR_CONF_CONTINUOUS.
| devc->continuous_mode = TRUE; | |
| devc->continuous_mode = FALSE; |
|
Hi @taorye I’ve been debugging timeout issues with the Sipeed SLogic Combo 8 and noticed that the driver currently lacks continuous mode support. I’ve added it in this PR. Could you please review the changes, especially the handling of SR_CONF_CONTINUOUS in the config getter/setter and the acquisition start logic? I want to make sure the implementation aligns with libsigrok’s expected behavior for continuous captures. Thanks! |
Add continuous mode support to Sipeed SLogic Analyzer
continuous_modein the device context structure.