-
Notifications
You must be signed in to change notification settings - Fork 185
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
Convert work #135
Convert work #135
Conversation
No kidding. I felt the same which I why I put it on pause
My thoughts that there would be a few common "native" formats that get widely used, particularly on the devices that use complex 8-bit streaming, since theres very few combinations, so they will probably get a lot of re-use. (CS8, CU8 for example). There could be some other "native" formats that seem more odd, but get a lot of re-use, and they would need a well known name. For example going between a complex stream on the PC and a packed two channel buffer would apply to a lot of dual channel RFICs. Like "CS16_x2" However, to handle this case, the converter function pointers would need to operate on a list of pointers (similar to the read/writeStream). And that might be good to do anyway. And then on the other hand, there are truly one-off native formats which are going to be highly hardware specific. I was thinking in this case, 1) we could not register them, or 2) they would need a hardware-specific name, probably the device name appended to them. But I think there are a few interesting points to 1). Registering converters, even native ones, regardless means that:
In some cases the hardware is going to support more than one format over the bus. If the user requests a specific bus format and the hardware supports, then the setupStream() implementation will have to do two things:
SoapyRemote basically does this with the
I feel like I have done that before in one project or another. IMHO thats actually an incredibly useful way to cleanup the converter for loops. One bonus, if anyone decided to implement/reimplement some of the converters in SIMD, those functions would be useful as well for the tail cases. So an installable public header with inline single operations conversions sounds pretty good. |
What if (say) WIRE was a defined Soapy format, but it was special in that it wasnt a hard definition such as CU8 or CF32 nor would it be exposed in get(Native)StreamFormats()? IE its format was device dependent, and could even change on the whim of the device and its Soapy driver. If the wire format changed, the driver would re-register all the WIREtoXXnn functions with the routines appropriate for the new wire format. Do i understand right? from an application viewpoint, getStreamFormats() tells me what grandiose formats are available from the next way-point, be it the device or a SoapyRemote, while getNativeStreamFormats() tells me what possible formats are efficiently available from the endpoint/device (ignoring buffer encoding etc) and would allow me to choose smartly. If so, then i would think that the device/driver would be best able to choose the wire format. (as an aside, are there hooks available in SoapyRemote to allow it to act as a sample rate up/down converter in addition to a format converter?) From my usage scenario only, I see the real power in Soapy as a defined framework of functions that i can flesh out to get my hardware to play in a bigger world. IE someone with more SDR experience than I has determined that these things are important and is what i should focus on. That framework doesnt necessarily minimize the trial and error, but sure cuts down on number of time i have to start over. The three things i seem to be struggling with (today) are a plethora of formats and their conversions, extracting a random mix of channels from the low level device buffer into a Soapy stream, and maintaining the leftover fragments without corruption. Coming up with a Standard Operating Practice for formats would be quite helpful. |
emulates acquireReadBuffer() and readStream(). creates a unique device buffer with each acquireReadBuffer. dumps buffers to help see interleave mistakes etc.
I added a device buffer simulator to the devel folder. The simulator allows one to create a fictitious device buffer and pass it thru rough approximations of acquireReadBuffer() and readStream(). Soapy's registered converters are used for buffer copying and exercising the convert primitives, some of which are so primitive they seem laughable. The simulated device buffer has the iteration, channel, and sample encoded in the data. When buffers are dumped and viewed, off-by-one errors etc will hopefully be fairly obvious. I also tried playing with structures for the complex elements. I dont have the background to know what problems they may create when crossing architectures or compilers. Same for the multidimensional arrays that i used. Any wisdom offered up would be appreciated. |
getNativeStreamFormat() was meant to return a well known format that was lossless, and not necessarily the underlying packed format. So if the DMA transfer gave you chunks of packed 3 byte complex samples, you would probably have implemented some getStreamFormats() like SC8/12/16 getNativeStreamFormat() would return SC12 because that can be used by default without any bit loss. -- but of course, there are stream args when the user needs to override this for BW reasons. So getNativeStreamFormat() could return any custom string for a custom format, and as long as SoapyRemote on the PC/client side knew about it, that would work. But the intention was really to only use a well known format, just one that matched the native DMA format in size, but maybe not packing. And of course, the server/driver side is usually going to be capable of working with this conversion between native DMA and well known stream, since you basically have to implement that anyway.
I hadnt thought of that. I imagine that most devices just pack things into the same byte multiple no matter what, but you definitely could come up with an optimization there. One thing that could be done is to just change the value returned by native stream format based on the current sample rate. Its probably too complicated to change the rate and packing on fly for a lot of other reasons as well. It basically means that setupStream just uses the DMA format that makes sense for the current rate, and the driver can of course issue a log warning if the rate is changed while streaming. We are basically asking the user to setup the stream after the rate and to tear down the stream if the rate changes first. Its funny but many of the SDR drivers themselves do something like this internally since many devices at a low level are not robust against rate/clock/and other similar changes.
I think that the driver should just register all of its functions, but giving them a name based on what that format is. So there is nothing to reregister, but just another string name for another format to lookup |
access map using multi dim array notation to better show intent.
Shows a simple model of stream formats and conversions.
The simulator should now show a rough, top down example of getting and setting stream formats and the selection of conversion routines using your Convert.hpp interface. At this point the primitives and conversion routines are a mess and should probably be my next focus. Any thoughts on naming would be helpful. I have no experience with SIMD. If after reviewing the work so far you feel it is still worth pursuing, maybe you could point me to an example library or document that would help in coding the primitives in an SIMD compatible way. |
No worries. I wouldnt even consider it for this merge. SIMD tends to bring in some new compiler issues and optional dependencies, so anything like this would be another module most likely. The only consideration for this was probably adding a priority to the registration so someone could provided a better converter and let the library select the one with higher priority. SIMD support usually means using compiler intrinsics for a SIMD core like AVX or SSE and creatively selecting calls to load chunks of memory, convert them, and store them back into some other memory. Its fairly detail oriented and already having good C implementations is perfect since we have something to compare to. Looks something like this
I think that I may have forgot about this question. For the most part the complex format on the PC is just an array of std::complex, numpy complex types in python, or pretty much anything where the real element is first in the struct and the imaginary element is second. So thats pretty well defined. Since any two well defined stream formats like CS16 and CF32 are just using the same ordering for I and Q, they can basically just use the same converter for the S16 and F32 format, just over twice the number of elements. In terms of how complex is represented in an arbitrary wire format, theres just no standard because it just comes down to someone's ASIC/FPGA byte packing. A lot of devices have some kind of byte shuffle support in them, so you can often organize the bytes in such a way that its in the same order that the convert expects. Example LMS7 LML interface supports arbitrary positioning of CHA/B I & Q within two DDR clock cycles. Or RTLSDR and other 8 bit format devices have an IQ swap option. |
lib/Convert.cpp
Outdated
{ | ||
std::vector<std::string> sources; | ||
|
||
for(SoapySDR::SourceFormatConverters::iterator it = Converters.begin() ; it != Converters.end(); ++it) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++11 is your pal here. :-)
You can cut something down into a ranged for loop like so:
for (const auto &it : Converters)
{
//it.first, it.second to access
}
devel/convertPrimatives.hpp
Outdated
@@ -0,0 +1,139 @@ | |||
// SPDX-License-Identifier: BSL-1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you happen to add to any particular file, add your john hancock "// Copyright (c) 2017-2017 Coburn Wightman" just like convert.cpp. Its the same license, but any number of copyright holders is fine.
include/SoapySDR/Convert.hpp
Outdated
#include <cstddef> | ||
#include <stdint.h> | ||
#include <stdexcept> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+#include <SoapySDR/Formats.hpp>
#include <vector>
#include <vector>
+#include <map>
#include <string>
#include <string>
+#include <cstring> //memcpy
#include <cstddef>
#include <cstddef>
+#include <stdint.h>
+#include <stdexcept>
Keep the headers to a minimum. Like if they are only used inside the .cpp for implementation reasons, they should only be included there. Unless of course the a particular definition is actually used in the said header file
Seems pretty good. I left a few comments. devel/ subdirectory is probably a good idea for the moment. I remembered now that I was going to have a static initialized object or static block that registers functions at static initialization time. And logs any errors of course because we cant be throwing in init. That way the new functions can be built into the library and also future modules can also do the same to add new converters. Thinking out loud we could do a registry class just like for the device registry.hpp
|
the devel folder is just a crutch. I will start tinkering on the converter registry class. I assume the class would be in lieu of the converter registry functions, ie the functions would not be intended to stand on their own? (--edit-- It seems listFind and listMake are defined outside the DeviceRegistry class. Would it make sense for the ConverterRegistry constructor to be overloaded so that when a formatter function pointer is supplied, that function is registered, without a function pointer the requested formatter is selected from the registry?) are you are still thinking of having a priority assigned to each converter? if so, would lesser converters get overwritten, or would you expect to get a list of converters of the same type with their respective priorities to choose from? Would the priority have any meaning other than lessor or greater? |
We could remove the registerConverter() from the public API. But I think its fine. In general the ConverterRegistry class will perform the registration, calling registerConverter() in its constructor. Keeping around registerConverter() as a public API is fine, it certainly doesnt hurt anything, and it may be useful for some advanced use case not yet imagined.
I think the priorities are basically an enum where the larger value is higher priority:
I cant think of anything else at the moment. |
The registry functions have been converted to a ConverterRegistry class. The class supports priorities and uses the Soapy Logger system for most errors except getConverterFunction where it logs and raises an exception rather than return a null pointer for a non existent converter. From my standpoint as a soapy user the class seems like it may be quite usable, but my grasp of what happens during Soapy's 'static initialization' and subsequent interaction with it is shaky. Does the ConverterRegistry have any relevance in the proposed Streamer API of #126 ? |
Looks pretty good, other than some cleanup (I can leave some inline comments). What do you think of
Seems ready to ship at that point!
Basically, soapysdr loads the modules with dlopen/loadlibrary. At this time, anything in the module that is declared static gets run. So ConverterRegistry() instances that are declared static get run at this point, which loads the registry. Did you see the various registration.cpp files in the Soapy* support projects, at the bottom they all instantiate a static registration object which basically does this, but for device enumeration and construction handles.
I was thinking that mostly it would be the current stream API but with for loops to make applications simpler on the user and also passing around an internal device pointer within the stream. But i suppose that we can now support formats in streams that would otherwise not support them! And keeping in line with the existing proposals ideology, the application layer is easier (more stream formats) and the driver author doesnt have to change anything. |
Initializes the converter registry and preloads with a base set of converter functions durring Soapy loadModules().
Added a converterRegistry module which initializes the registry using the DefaultConverters constructor. notes:
|
I think I see the problem. So you dont really need a default constructor or reference counting for the ConverterRegistry. The constructor simply adds the entry and the destructor simply removes it. I think the main trouble came from std::vector of these classes which was making copies and using default constructors. The DefaultConverters should become a single .cpp file in the library that just contains lines like this at the bottom:
So DefaultConverters.cpp is pretty much correct with genericCU16toCF32 and similar implementations but I would suggest:
The fact that its compiled into the main library will mean they are there when the library itself is loaded, and be around for things like unit tests. So that covers all of the files in devel/ and convert/ besides the unit tests. I suppose the unit tests could go into tests/ and be run just like the other units tests (like in |
Default converter functions would be compiled into Soapy's Converter Registry with:
A driver could register a converter function:
A driver would access the registry with:
and use a registered function with:
I took your thought in #135 (comment) to mean to abandon the Converter.cpp and Converter.hpp interface altogether. I worry now that wasn't the case and would be happy to revisit. |
@coburnw Let me know if I missed some use case, but I was thinking of the use case being a lot like how the device registry is used so:
yup
Whats the motivation for this being different than the previous case (ie built into the library)? The thought was that there is one way to register them (static initialization). And one way to access them: Its sort of like the drivers only know a converter by its string format names, but the converter itself could be encapsulated into the driver, library, or even another module.
Oh, maybe this caused the confusion. Basically, except that I thought that these functions were static members. So you dont make an instance of ConverterRegistry to access the registry. Its more like I dont know if this got dropped or I forgot it in the mockup class long ago. But its kind of like this: there is just one global registry, so ConverterRegistry is almost like a namespace in this regard when it comes to the methods.
Probably my fault, ConverterRegistry is likely basically things 1) a registry liftetime management of a convert function and 2) a static class of methods to access a global registry and I conflated them while talking. In fact if they were two different classes ConverterRegistryGlobal with only static methods to access the registry, and ConverterRegistryEntry for making static init registry entries and function lifetime management, it would have a tinge of less ambiguity :-P But it works FWIW I think the addition of |
I think these commits are more in line with your intent. |
Other than some debug prints, it seems good to me. I think that we can always add more converters as an afterthought/future PRs, etc |
I think there is a reasonable set of default complex converters defined. for clarification, am i correct in these two points:
If the converters are on the right track, ill duplicate the same set of formats for real values, test and cleanup, and call it done. |
I wanted to be complete and support unsigned numbers. But In practice I haven't seen this, except a few devices use complex uint8 and force you to subtract out 127 to rescale them to a signed value. RTL Streaming:
Simply casting between the types works to perform the conversion. So you only have to scale up or down before hand. |
I think this is pretty close to the original intent outlined in Convert.hpp although it lacks a default argument for scale factor. Since optional arguments and function pointers dont play well in C++, it seems a default scale factor will probably require a functor or similar. In addition, the plethora of defaultConverters is crying for a template but my first stab at this is implying the ConverterFunction typdef will need to be modified, which i think breaks the original definition. I also worry that templates could complicate things for a Soapy device programmer that just wants to add a converter or two. I would be happy to explore a functor, template or some other concept further. What parts of this should i pursue, or should we call it good? |
In practice, SoapyRemote or newer Soapy Stream API will get the scale factor from If someone was manually invoking the converters inside of readStream/writeStream; typically this would be float to or from native format, so they also know the scale. I suppose for testing we need to know a default scale factor between floats and signed integers, and between types of integers. Mostly for unit testing purposes though, I think? So just like format.hpp has
I can see a template being helpful, you can just register the template specialization while letting the template actually figure out which elements to convert. Even if some users may not go for it, could be nice either way.
Does keeping the scale factor everywhere consistently solve the problem? Scale factor is just something the top level invoker functions would have to pass in |
A possible buffer format conversion implementation for Soapy's Convert.hpp.
see issue #49
This has been a struggle for me as my focus is from the native sample format, and i am getting the sense of how this could get messy.
Was adding a generic 'native' format to Formats.hpp part of the thought outlined in #49? if so, then a NATIVEtoCF32 function could be registered by the driver. On the other hand, perhaps native format has no business being a registered function. Related, how does WIRE format fit into this?
Another thought was writing a group of efficient, tested, inline functions that convert a single sample from format to format. Those could then be inserted into both the Soapy registered buffer conversion functions and also the driver's low level buffer decoder.