e3xx: Add support for power calibration API

[draeman-synoptic]

This adds support for the power calibration API to the E3xx radio family (e.g. the set_[tx|rx]_power_reference() API calls). It also includes support within the uhd_power_cal.py utility script and updates the E3xx documentation.

Description

This PR primarily updates the e3xx radio_control impl to include support for the pwr_cal_mgr interface. This allows the radio family to support the set_[tx|rx]_power_reference() API calls. The python utilities were updated to include a "calibrator" subclass for this radio family, so that the existing uhd_power_cal.py utility can be used to generate calibration tables for these radios. Finally, the e3xx documentation was updated to mention support for this API, similar to the blurbs that exist in the B200/X3xx documentation.

Because the pwr_cal_mgr accesses the radio_control_impl's gain setter, the set-lock used by the e3xx_radio_control_impl was changed from a mutex to a recursive_mutex. This allows the gain update to occur while processing the frequency change, without releasing the lock in between. I noted that recursive mutexes are already in use across the codebase, including in other radio_control impls.

Implementation Note!

One observation to consider: calibration data is generated and uniquely identified per antenna, per direction, per channel. This means that for RX on a given channel, the antennas RX2 and TX/RX are expected to have separate calibration tables. None of the existing devices that support power calibration API seem to handle this correctly - they all create an instance of pwr_cal_mgr identified by the default antenna in each direction, and that table gets used for any port. For example, on most (all?) devices, the receive cal table for "RX2" will be used regardless of whether "RX2" or "TX/RX" is selected, despite the fact that generation is implied to be antenna-specific.

In this patch, I deviated from that behavior and tried to respect that independent tables are maintained for each of the two RX antenna ports per channel on the E320. Instead of just instantiating one pwr_cal_mgr per direction per channel, like other devices do, I instead create one per antenna per direction per channel. Then I reference into the correct manager if the antenna selection is changed. However I am currently not maintaining the power-tracking across antenna change, although perhaps that would be desirable.

If adhering to separate cal table per antenna is not desirable, then the patch should be changed to be more in line with other impls. However, in that case, I'd also suggest updating the calibration generation and keying to reflect that. Or, perhaps an approach where antenna-specific data is preferred if available, otherwise fallback to only use the default antenna data.. ?

Which devices/areas does this affect?

The change only impacts E3xx radios.

Testing Done

I confirmed that I can generate TX calibration tables for an E320 using the existing uhd_power_cal.py, those tables are referenced appropriately by the E320 device, and it tracks TX power levels after I call set_tx_power_reference(). Further, I confirmed that gain settings are updated correctly upon re-tune, that each TX channel is tracked separately, and that manually setting tx_gain will exit power-tracking mode. Functionality was confirmed by querying the tx_gain setting after re-tune, and also viewing transmit power levels on a spectrum analyzer.

I did not test RX power tracking (which I don't use), nor did I test any radio in the E3xx family beyond the E320.

Checklist

• [x] I have read the CONTRIBUTING document.
• [x] My code follows the code style of this project. See CODING.md.
• [x] I have updated the documentation accordingly.
• [ ] I have added tests to cover my changes, and all previous tests pass.
• [ ] I have checked all compat numbers if they need updating (FPGA compat, MPM compat, noc_shell, specific RFNoC block, ...)

[mbr0wn]

Many thanks. This will take a while before we get it reviewed, tested, and merged.

[draeman-synoptic]

Sure, though it'll be a few days until I can circle back to it. Any objection if I split that third "the rest" patch into two commits? (3a) would add pwr_cal_mgr support similar to existing impls, with one table per channel per direction. (3b) would add support for per-antenna tables, which lines up better with existing documentation but diverges from the existing impls (at least on the 300-series radios). I'm thinking it might be easier to review and incorporate (3a) by itself, then separately you can consider what you want to do regarding (3b).

[mbr0wn]

That's a great idea, yeah!

[mbr0wn]

@draeman-synoptic Thanks for splitting this up. I will start feeding the first two commits into master ASAP. If they go public before we merge this, then maybe you can rebase.

I have yet to do a more thorough review, but I still don't think we need the fourth commit. I would have sworn that B200 and X300 resolve the antenna dynamically, and the code looks like it does, too, but I don't have all the gear to verify that right now.

[atrnati]

A couple of comments before I skedaddle.

I'm wondering if it would be possible to implement this without the need to change the setter lock to a recursive mutex. When I see one in use, it sets my spidey sense tingling, as I've always followed the maxim that, to paraphrase Dijkstra, recursive locks are considered harmful--see, for example, this Google search--and they set off some well-earned PTSD from my many years of Windows kernel driver debugging where recursive mutexes were concerned.

Because the pwr_cal_mgr accesses the radio_control_impl's gain setter, the set-lock used by the e3xx_radio_control_impl was changed from a mutex to a recursive_mutex.

I'm wondering if it would be possible to factor out the code in the gain setter that ends up getting called in this sequence into an internal private _set_..._gain() function which assumes the set lock is held. That function could be called under the non-recursive lock from both pwr_cal_mgr and the setter.

That said:

• I'm not an expert in this part of UHD.
• Apparently, other device types' radio_control classes already are using the recursive mutex, so its use in this type of situation isn't unprecedented.
• Making this change probably means refactoring a bunch of other code too, and introducing additional risk.

So in this case, I'm okay with making an exception for the aforementioned reasons.

[draeman-synoptic]

@mbr0wn - Thanks for the input! I agree that the 4th patch is not necessary - I overlooked that other impls were querying the active antenna within the body of the lambda expression - that makes a lot more sense. What's the best way to proceed? I can fix the lambda expressions in my third patch, and we can drop the fourth patch.

@atrnati - Thanks for the feedback on the recursive mutex - I completely agree recursive locks make it much more difficult to trace the synchronization "correctness" of a complex codebase, and can be a sign of excess complexity. I like the suggestion of just refactoring set_..._gain() code into two methods - the private helper method does the work, and the public API method is just a shim that grabs the (non-recursive) mutex and then calls the helper. Unfortunately it looks like the change to a recursive mutex was merged and shipped in the 4.2.0.1 release, so maybe that ship has sailed.. And as we discussed, there are recursive mutexes already used in other radio impls. I'm happy to go either way on this - just let me know.

[draeman-synoptic]

Hi, I've made the following updates to this PR:

1. Rebase to master (as of 4.3.0.0 release)
2. Make the simplification discussed above - get the current antenna within the body of the lambda expression, which ensures the proper antenna-specific cal file is used. I also did some light testing and confirmed this works as expected.
3. Added a call to update_power() when RX antenna is set - this is necessary to properly handle the case where a UHD app calls set_rx_power_reference() and then subsequently calls set_rx_antenna().
4. Run clang-format

Let me know if anything else is needed here.

[mbr0wn]

@draeman-synoptic Hey, sorry we dropped the ball on this. I'm still interested in getting this into the next release.

[mbr0wn]

I've thrown this into our internal CI/review pipeline. Many thanks again, and thanks for your patience!