feat: add pattern generator SCPI commands - #202
Conversation
Reviewer's GuideAdds a complete SCPI-facing digital pattern generator, wiring the new pattern generator core into the protocol stack, introducing a mid-layer controller, and a PIO/DMA-based low-level pattern output engine, plus integrating periodic servicing and reset into the existing application framework. Sequence diagram for SCPI PG:STARt command pathsequenceDiagram
participant Host
participant SCPI_Server as SCPI_Server
participant PG_SCPI as scpi_cmd_pattern_generator_*
participant PG_App as pg_*
participant PatternGenerator as pattern_generator_*
participant PatternOutputLL as pattern_output_ll_*
Host->>SCPI_Server: PG:DATA <block>
SCPI_Server->>PG_SCPI: scpi_cmd_pattern_generator_data
PG_SCPI->>PG_App: pg_upload_data
PG_App-->>PG_SCPI: bool
Host->>SCPI_Server: PG:STARt
SCPI_Server->>PG_SCPI: scpi_cmd_pattern_generator_start
PG_SCPI->>PG_App: pg_start
PG_App->>PatternGenerator: pattern_generator_start
PatternGenerator->>PatternOutputLL: pattern_output_ll_start
PatternOutputLL-->>PatternGenerator: bool
PatternGenerator-->>PG_App: bool
PG_App-->>PG_SCPI: bool
PG_SCPI-->>SCPI_Server: SCPI_RES_OK / SCPI_RES_ERR
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The pin/rate validation logic is duplicated with slightly different constraints between the SCPI layer (e.g.
PG_MAX_PIN_COUNT,PG_MAX_RATE_HZ,config_is_validinpattern_generator_commands.c) and the system layer (config_is_validinpattern_generator.c); consider centralizing these constraints to a single place to avoid drift and make future changes safer. - In
pattern_output_ll_inityou unconditionally setbus_ctrl_hw->priorityto DMA RW priority, which globally affects the bus for the whole system; if other subsystems rely on different priorities it may be worth capturing and restoring the previous value or moving this to a more global/central init path instead of doing it per-pattern-generator instance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pin/rate validation logic is duplicated with slightly different constraints between the SCPI layer (e.g. `PG_MAX_PIN_COUNT`, `PG_MAX_RATE_HZ`, `config_is_valid` in `pattern_generator_commands.c`) and the system layer (`config_is_valid` in `pattern_generator.c`); consider centralizing these constraints to a single place to avoid drift and make future changes safer.
- In `pattern_output_ll_init` you unconditionally set `bus_ctrl_hw->priority` to DMA RW priority, which globally affects the bus for the whole system; if other subsystems rely on different priorities it may be worth capturing and restoring the previous value or moving this to a more global/central init path instead of doing it per-pattern-generator instance.
## Individual Comments
### Comment 1
<location path="src/application/pattern_generator_commands.c" line_range="43-44" />
<code_context>
+
+static bool apply_config(void)
+{
+ if (!config_is_valid()) {
+ return false;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align configuration validation limits with low-level constraints to improve error consistency
`config_is_valid` only enforces `rate_hz <= PG_MAX_RATE_HZ`, while `pattern_output_ll_configure` additionally requires `clk_div >= 1.0f` based on the actual peripheral clock. As a result, some rates that pass this check will still be rejected later with less clear SCPI errors.
To make failures consistent and predictable, either derive `PG_MAX_RATE_HZ` from `PLATFORM_get_peripheral_clock_speed` (or a documented worst-case), or replicate the `clk_div >= 1` constraint here so all invalid configs are rejected at the same layer with a clear, shared limit.
Suggested implementation:
```c
static bool config_is_valid(void)
{
/* Ensure configuration constraints match low-level limits so invalid
* configurations are rejected consistently before calling into the
* pattern_output_ll layer.
*/
const uint32_t peripheral_clk_hz = PLATFORM_get_peripheral_clock_speed();
const float clk_div = (float)peripheral_clk_hz / (float)state.rate_hz;
return state.pin_count >= 1 &&
state.pin_count <= PG_MAX_PIN_COUNT &&
state.pin_base + state.pin_count <= 30 &&
state.rate_hz >= 1 &&
state.rate_hz <= PG_MAX_RATE_HZ &&
clk_div >= 1.0f;
}
```
```c
#include <string.h>
#include "system/pattern_generator.h"
#include "system/platform.h"
```
To keep this check perfectly aligned with `pattern_output_ll_configure`, ensure that the `clk_div` computation here matches whatever that function uses (e.g., if it divides by additional factors such as prescalers or pattern word widths, mirror that logic instead of the simple `peripheral_clk_hz / state.rate_hz` used above). If `PLATFORM_get_peripheral_clock_speed` is already declared via another header in this translation unit, you can omit the additional `#include "system/platform.h"` to avoid redundant includes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (!config_is_valid()) { | ||
| return false; |
There was a problem hiding this comment.
suggestion (bug_risk): Align configuration validation limits with low-level constraints to improve error consistency
config_is_valid only enforces rate_hz <= PG_MAX_RATE_HZ, while pattern_output_ll_configure additionally requires clk_div >= 1.0f based on the actual peripheral clock. As a result, some rates that pass this check will still be rejected later with less clear SCPI errors.
To make failures consistent and predictable, either derive PG_MAX_RATE_HZ from PLATFORM_get_peripheral_clock_speed (or a documented worst-case), or replicate the clk_div >= 1 constraint here so all invalid configs are rejected at the same layer with a clear, shared limit.
Suggested implementation:
static bool config_is_valid(void)
{
/* Ensure configuration constraints match low-level limits so invalid
* configurations are rejected consistently before calling into the
* pattern_output_ll layer.
*/
const uint32_t peripheral_clk_hz = PLATFORM_get_peripheral_clock_speed();
const float clk_div = (float)peripheral_clk_hz / (float)state.rate_hz;
return state.pin_count >= 1 &&
state.pin_count <= PG_MAX_PIN_COUNT &&
state.pin_base + state.pin_count <= 30 &&
state.rate_hz >= 1 &&
state.rate_hz <= PG_MAX_RATE_HZ &&
clk_div >= 1.0f;
}#include <string.h>
#include "system/pattern_generator.h"
#include "system/platform.h"To keep this check perfectly aligned with pattern_output_ll_configure, ensure that the clk_div computation here matches whatever that function uses (e.g., if it divides by additional factors such as prescalers or pattern word widths, mirror that logic instead of the simple peripheral_clk_hz / state.rate_hz used above). If PLATFORM_get_peripheral_clock_speed is already declared via another header in this translation unit, you can omit the additional #include "system/platform.h" to avoid redundant includes.
There was a problem hiding this comment.
Copilot flagged the following:
- Rebase onto #199 before merge.
CloudyPadmal
added
the
conflicts
4ddd628 to
609ea52
Compare
|
Rebased and fixed conflicts |
CloudyPadmal
removed
the
conflicts
| enum { | ||
| PG_DEFAULT_PIN_BASE = 16, | ||
| PG_DEFAULT_PIN_COUNT = 1, | ||
| PG_DEFAULT_RATE_HZ = 1000, |
There was a problem hiding this comment.
Wouldn't this default rate conflict with the clk_div min check?
There was a problem hiding this comment.
yes, ive fixed it now
| state.pin_base = pin_base; | ||
| state.pin_count = pin_count; | ||
|
|
||
| if (pg_initialized && !apply_config()) { |
There was a problem hiding this comment.
Shouldn't pg_initialized be set to false here?
There was a problem hiding this comment.
I have made changes to set it to false
CloudyPadmal
left a comment
CloudyPadmal
left a comment
There was a problem hiding this comment.
Do you have any test results from the max rate @ the advertised 75 MHz?
CloudyPadmal
left a comment
CloudyPadmal
left a comment
There was a problem hiding this comment.
I just noticed that there is no README update for PG commands
|
added readme docs for the commands |
|
@CloudyPadmal about the test results. I can verify it works properly until around 25 Mhz. This is using the pslab pico's logic analyser and pulseview. 
While I can see the waveforms at 75 Mhz, the timings and shape of waveform is a bit off, my guess would be this is because I am sampling at 150 mhz and the signal itself is 75, we are not sampling at the correct required rate. Could also be due to amateur wiring quality worsening signal quality at high frequencies. The PG works at 75 mhz, just that I can't verify the timing since I don't have access to a proper faster logic analyser. |
|
I would like to take a second look at The number itself is consistent as 150 MHz sysclk/2 instructions per sample gives 1.0 clock divider, so the config is accepted. The problem is what happens after that. The PIO does a What concerns me more is that we won't know when it doesn't. |
|
Okay yes, that makes sense. How would you suggest I move forward? |
|
I think the latter would be the best |
|
okay, I will work on this. I think the pr would become bulky if made the changes in this one. Should I make another PR after this one gets merged? |
|
Since I'm a bit familiar with this PR, let's do the changes here. It'll be easier to go through. |
|
@CloudyPadmal so I have implemented packed pattern samples. This means one Allso added explicit PC reset on start() so repeated starts execute the initial PULL. Also implemented underrun tracking by: Also added |
Adds the SCPI/application layer for the digital pattern generator introduced in #199.
This PR wires the pattern generator core into the SCPI command table and adds the protocol handlers needed to configure/start/stop pattern output from the host.
This is meant to be merged after #199 and thus is stacked on top.
Summary by Sourcery
Integrate the new digital pattern generator into the SCPI protocol stack and platform, providing host-configurable pattern output over PIO/DMA.
New Features:
Enhancements: