NCO Frequency cannot be set with LIQUID_VCO_DIRECT
jgaeddert opened this issue · 4 comments
The user can specify nco_crcf_create(LIQUID_VCO_DIRECT) but then later cannot set the frequency using the common interface nco_crcf_set_frequency(q, 0.12345f). The frequency can only be set with an internal method nco_crcf_set_vcodirect_frequency() which is not exposed to the user.
Tagging @ArtemPisarenko.
Issue isn't clear for me. nco_crcf_set_vcodirect_frequency() method isn't internal. It's exposed to user. Otherwise object would be non-functional.
Did you mean that NCO objects created with different types have different methods for setting/getting frequency? If so, then it's a choice of whole library design considerations. Should same module objects have fully identical API (allowing different sub-types to change only internal behaviour and characteristics)? If so, LIQUID_VCO_DIRECT nco should be reimplemented as separate module. If library would written in C++ then common part of API could be separated to base class (NCO interface) and specific module classes (NCO types) could be derived from it. But it's C lang. So I decided the most flexible design option is to add "vco-direct" implementation as additional run-time type and differentiate API partially. Because this kind of NCO have more common than differences. Its specifics cannot be hidden from user. There are no universal optimal way to convert arbitrary floating-point dtheta value to the pair of n and m integer values. Also it's not possible to change phase arbitrarily during stepping (although current implementation might be improved to specify starting phase, i.e. initial phi floating-point value). So, user should be aware of such limitations, i.e. LIQUID_VCO_DIRECT is application-specific. It's a price user must pay for achieved NCO characteristics.
Ok, my apologies. There are two things going on that are related. You're correct that the user can set the frequency of LIQUID_VCO_DIRECT with the nco_crcf_set_vcodirect_frequency() method. My apologies; when I rebased your branch onto master I somehow missed this in the global header. Thank you for reminding me of this.
The second (which was the origin for this issue) is that there are now separate methods for getting/setting frequencies based on types. As you point out, this is a choice of library design considerations. I really see one of two options for how the NCO functionality should work:
- There is one object that can be created several ways at runtime, but with identical interfaces
- There are completely separate objects with separate interfaces
My preference would be for option 1, but I understand your reasoning for choosing the interface you did.
Currently, we have separate functional objects packed into one: nco_crcf, but separate interfaces for them after instantiation.
In my opinion, the nco_crcf object should have a fixed-point (e.g. 32-bit) phase representation because its phase and frequency can be adjusted extremely quickly using integer operations and masking. Then the implementation for the type (look-up table, LUT with linear interpolation, etc.) can be defined by LIQUID_NCO or LIQUID_VCO etc. If we want an NCO-like object that doesn't support this functionality, I think it makes sense to have it as a separate object.
What are your thoughts?
(p.s. thank you for supporting this conversation; I've found it very useful!)
I haven't any preferences about design choices, because I haven't used the library for a long time and hardly remember things I did :)
I agree that phi and theta values would be better represented as fixed-point (both interface and internals), but it has nothing to do with functionality. LIQUID_VCO_DIRECT algorithm is natural fixed-point, but it still differs in what nco_crcf methods are supported. So these are two separate issues.
If you prefer option 2, extracting current LIQUID_VCO_DIRECT implementation to separate module would be pretty straightforward. I mean that it's an easy and trivial work compared to design decisions we discuss.
The only important thing I see is how much user be happy with limitations of current LIQUID_VCO_DIRECT implementation. Some limitations can't be either avoided at all, or some non-trivial compromises could be found (e.g. setting phase during stepping). Some ones aren't actually limitations but rather missing trivial features I didn't care about when implemented for my needs (e.g. defining initial phase or getting phase). I have time to discuss, but I'm not sure I'll be able to support my commitment further.