gnuradio / gnuradio

GNU Radio – the Free and Open Software Radio Ecosystem
https://gnuradio.org
GNU General Public License v3.0
5.12k stars 1.91k forks source link

Allow Soapy message commands to set non-channel-specific device settings #6604

Open ryanvolz opened 1 year ago

ryanvolz commented 1 year ago

Feature Description

I have been bit by the following TODO for gr-soapy's block_impl::cmd_handler_setting function:

// TODO: any way to call channel-less version?
void block_impl::cmd_handler_setting(pmt::pmt_t val, size_t channel)

Basically I want to send a "setting" command to a Soapy Source block to toggle a bias tee, but that is not currently possible because the setting in question ("biastee") is a device setting that could be set by the channel-less version of write_setting but the command handler only has the ability to call the channel-specific version of write_setting where "biastee" is not valid. Currently the only way to use these channel-less settings is by specifying them as device arguments during initialization.

Note that not setting a chan key in the command causes the current command handler to loop through all active channels and apply that command to each of them, which is where this gets forced into channel-specific code paths in the first place. For other command handlers this works out OK in the end because even though all of the command handlers take a channel argument, a lot of them just ignore it as irrelevant. The "setting" command seems somewhat unique in that the Soapy API defines both channel-specific and channel-less settings, although in practice a quick survey of Soapy device modules didn't turn up any that use channel-specific settings.

I can imagine different ways of implementing this:

  1. It's Soapy's problem, the channel-specific settings should fall back to channel-less settings at the Soapy API level.
  2. It's gr-soapy's problem, the channel-specific write_settings should fall back to the channel-less write_settings in the gr-soapy block implementation.
  3. Explicit is better than implicit and we should avoid fallbacks, so a "setting" command without a channel specified should go to the channel-less write_settings and not loop through all of the available channels. This requires changing the message command handler, which has implications for all other commands. This opens up even more possibilities.

Thoughts?

Feature Urgency

medium (would be nice to have in the near future)

More Information

No response

willcode commented 1 year ago

How about it does both:

The pattern in the driver implementations seems to be ignore (or log in BladeRF) unknown settings. But since none of the drivers we see implement the channel version, the default will ignore all the un-needed calls.

willcode commented 1 year ago

I see the iteration over channels only at init. The handler always assumes there is a channel.

ryanvolz commented 1 year ago

I see the iteration over channels only at init. The handler always assumes there is a channel.

The individual command handlers always assume a channel, and it's the message handler that calls those that loops through all channels if none is specified: https://github.com/gnuradio/gnuradio/blob/627493488b365aa9ce2464bc059db6fa11757bd3/gr-soapy/lib/block_impl.cc#L1621.

willcode commented 1 year ago

Found it right after hitting send. Funny that CHANNEL_ALL is private.

willcode commented 1 year ago

Minimal change would be to add the channel-less call unconditionally right after the loop. That way, if anyone writes a channel-specific driver setting, it still works. The only thing driver devs need to worry about is a stray channel-less call, and it looks like that doesn't impact any of the drivers in the Pothos repos. Better to work for all the drivers we know about than a hypothetical fragile driver we don't know about.

ryanvolz commented 1 year ago

Do you mean putting another handler call in this else after the for loop https://github.com/gnuradio/gnuradio/blob/627493488b365aa9ce2464bc059db6fa11757bd3/gr-soapy/lib/block_impl.cc#L1617-L1625 that doesn't pass a channel or uses some sentinel channel value? (I don't know the best way to do that in C++.)

Or do you mean adding a channel-less write_setting after each existing write_setting in cmd_handler_setting?: https://github.com/gnuradio/gnuradio/blob/627493488b365aa9ce2464bc059db6fa11757bd3/gr-soapy/lib/block_impl.cc#L1482-L1488 But I guess that still runs into the throwing error condition in https://github.com/gnuradio/gnuradio/blob/627493488b365aa9ce2464bc059db6fa11757bd3/gr-soapy/lib/block_impl.cc#L1113-L1125 That's where I thought a fallback to the channel-less write_setting would make some sense instead of throwing.

willcode commented 1 year ago

The latter.

What happens when a throw is hit here? Just a logging message? Do you have a flowgraph? I can plug in a RTL and see for myself.

ryanvolz commented 1 year ago

The latter.

What happens when a throw is hit here? Just a logging message? Do you have a flowgraph? I can plug in a RTL and see for myself.

It aborts and stops the flowgraph. In my biastee test case:

..terminate reached from thread id: 7f44b5ffb640Got std::invalid_argument
Invalid setting: biastee
Aborted