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

Fix qa_Picoscope4000a regressions #136

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Fix qa_Picoscope4000a regressions #136

merged 1 commit into from
Jan 18, 2024

Conversation

frankosterfeld
Copy link

@frankosterfeld frankosterfeld commented Jan 17, 2024

Bump to latest graph-prototype and fix the regressions with Picoscope4000a by adapting to the new lifecycle API.

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);
Copy link
Author

@frankosterfeld frankosterfeld Jan 17, 2024

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??

Copy link
Member

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 &

Copy link
Author

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));
Copy link
Author

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)

Adapt Picoscope4000a to API changes, esp. the new lifecycle API.
Copy link

sonarcloud bot commented Jan 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@frankosterfeld
Copy link
Author

Test coverage cannot be improved here, as qa_PicoScope4000a requires the hardware to run...

Copy link
Member

@wirew0rm wirew0rm 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, 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.

@wirew0rm wirew0rm merged commit 68b8bfc into dev-prototype Jan 18, 2024
6 of 7 checks passed
@wirew0rm wirew0rm deleted the frank/ps-fixes branch January 18, 2024 15:54
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.

3 participants