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

Refactor Picoscope implementation #147

Merged
merged 6 commits into from
Jun 19, 2024
Merged

Conversation

vimpostor
Copy link

@vimpostor vimpostor commented Jun 12, 2024

As discussed, this implements all of the following items:

  • Restrict Picoscope value types to int16_t, float and gr::UncertainValue<float>
  • Remove the custom work() implementation and port Picoscope to use processBulk()
  • BlockingIO is now chosen at compile-time, based on whether the acquisition mode is streaming or rapidBlock
  • Generalize and extend the Picoscope Block so that it can be used for all Picoscope variants.
  • Incorporate timing messages received over the message port, when a trigger arrives
  • Update to latest GR version

@vimpostor

This comment was marked as outdated.

This fixes the build with gcc14.

Signed-off-by: Magnus Groß <magnus.gross@kdab.com>
This restricts the possible output types to:
- std::int16_t
- float
- gr::UncertainValue<float>

Signed-off-by: Magnus Groß <magnus.gross@kdab.com>
This allows the Block to be extended with support for various Picoscope
models.

Furthermore, this ports the custom work implementation to processBulk()
and implements the inclusion of timing messages received over the
message port.

Fixes #113

Signed-off-by: Magnus Groß <magnus.gross@kdab.com>
Signed-off-by: Magnus Groß <magnus.gross@kdab.com>
@vimpostor
Copy link
Author

I have updated to latest graph-prototype, but it looks like there is still an unnecessary copy-constructor that causes the build to fail.
The following patch should work:

diff --git a/core/include/gnuradio-4.0/BlockModel.hpp b/core/include/gnuradio-4.0/BlockModel.hpp
index 44f209d..d0a2aff 100644
--- a/core/include/gnuradio-4.0/BlockModel.hpp
+++ b/core/include/gnuradio-4.0/BlockModel.hpp
@@ -353,7 +353,7 @@ public:
 
     ~BlockWrapper() override = default;
 
-    explicit BlockWrapper(property_map initParameter = {}) : _block(std::move(initParameter)) {
+    explicit BlockWrapper(property_map initParameter = {}) : _block{{std::move(initParameter)}} {
         initMessagePorts();
         _dynamicPortsLoader = std::bind(&BlockWrapper::dynamicPortLoader, this);
     }

@RalphSteinhagen
Copy link
Member

The syntax is a bit weird:

_block{{std::move(initParameter)}}

because this implies that you create a map in a map... 🤔

@vimpostor
Copy link
Author

because this implies that you create a map in a map... 🤔

Not quite, I agree the syntax is a bit confusing, but this is to force list-initialization instead of copy-initialization according to [dcl.init.aggr] section 8.5.1 clause 2:

Each member is copy-initialized from the corresponding initializer-clause.
(...)
If an initializer-clause is itself an initializer list, the member is list-initialized.

So the extra {} is there to make the initializer clause itself an initializer-list, see also https://stackoverflow.com/a/60609943.

I was also a bit confused by this behaviour, but you can test it empirically: Without the patch applied the build fails, because the constructor is ill-formed. With the patch applied, everything works.

I hope that explains it. :)

@vimpostor
Copy link
Author

vimpostor commented Jun 14, 2024

@RalphSteinhagen I have pushed an alternative commit f615266, that works around this fishy behaviour by redefining the property_map constructor. Let me know if you prefer that over the patch described above.

This is needed as a workaround for very fishy behaviour, where GNURadio
tries to use an ill-formed copy-constructor.

Alternatively the BlockWrapper constructor could be patched to use
_block{{std::move(initParameter)}}, which forces direct
list-initialization.

Signed-off-by: Magnus Groß <magnus.gross@kdab.com>
Copy link

@drslebedev drslebedev left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR!

Overall, the changes look good.

I made a few minor updates and pushed them directly to your branch:

  1. Changed sample_rate to float (which triggered some other modifications).
  2. Replaced custom sink with TagSink.
  3. Removed requestStop and publishEOS in the processBulk function; just return DONE.
  4. Updated the processBulk signature.
  5. Add support to store UncertainValue samples for TagMonitor and TagSink Add support to store UncertainValue samples for TagMonitor and TagSink  gnuradio4#363

The streaming mode is functioning without problems.

However, there are two issues that need further investigation (but not for this PR):

  1. In RapidBlock mode, there's a data race when publishing the EOS tag.
  2. In RapidBlock mode, when nCaptures > 1, the output data is overwritten after publish(). The offset is not taken into account.

The PR can be merged.

Copy link

sonarcloud bot commented Jun 19, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@drslebedev drslebedev merged commit ba28ef2 into dev-prototype Jun 19, 2024
6 of 8 checks passed
@drslebedev drslebedev deleted the picoscope_refactor branch June 19, 2024 11:16
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.

[5pt,7pt] graph-prototype: Port and simplify picoscope implementations
3 participants