-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix qa_Picoscope4000a regressions #136
Conversation
69cc8f4
to
c889b38
Compare
const auto writeSignalInfo = !channel.signal_info_written; | ||
const auto tagsToWrite = triggerTags.size() + (writeSignalInfo ? 1 : 0); | ||
if (tagsToWrite == 0) { | ||
continue; | ||
} | ||
auto writeTags = channel.tag_writer.reserve_output_range(tagsToWrite); | ||
if (writeSignalInfo) { | ||
writeTags[0].index = static_cast<int64_t>(state.produced_worker); | ||
writeTags[0].index = static_cast<int64_t>(ps_state.produced_worker - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is writing the per-output signal info (SIGNAL_NAME, SIGNAL_UNIT, etc.). This is done manually b/c the block doesn't use the default work()/processBulk() implementation, and instead writes directly to the buffers. Since the latest changes, this only works with the "- 1", leading to an index of -1 instead of 0 when sent with the first data chunk. Is that correct??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1
is the correct index for the very first tag. Normally you wouldn't need to (and shouldn't) publish this tag explicitly but it's implicitly generated via the property map that is given to the make block routine of the Graph and/or the Block constructor or init(..) functions that take the settings as const property_map &
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have per-channel settings, and need to do it manually because of the custom work() function, so I think it's needed here. One could argue whether the index should be always -1 (wildcard).
const auto &tag = tagTracker.seen_tags[0]; | ||
expect(ge(sink.samples_seen, 80000UZ)); | ||
expect(le(sink.samples_seen, 160000UZ)); | ||
expect(eq(tagMonitor.signal_name, "Test signal"s)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't signal_name and sample_rate automatically get the values from the PS block, via the SIGNAL_NAME/SIGNAL_RATE tag values? (which are received by the monitor, see checks below)
0b0fd54
to
cef9566
Compare
Adapt Picoscope4000a to API changes, esp. the new lifecycle API.
cef9566
to
117e39c
Compare
Quality Gate failedFailed conditions 0.0% Coverage on New Code (required ≥ 80%) |
Test coverage cannot be improved here, as qa_PicoScope4000a requires the hardware to run... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, tested locally including the hardware tests.
I also tested the changes with fair-acc/opendigitizer#144, builds without problems. Running it there's still some problems, but I don't think they are likely to be rooted in gr-digitizers.
Bump to latest graph-prototype and fix the regressions with Picoscope4000a by adapting to the new lifecycle API.