lime suite: The IConnection overhaul about limesuite HOT 25 CLOSED

Work on virtual methods here: https://github.com/limemicro/lms7suite/tree/connection_develop

from limesuite.

Made a quick additional function list for the IConnection. These should cover most of the possibly needed operations IConnection.txt, it still needs refining.

I think there should be listing of not just RFIC, but all the IC that can be controlled. Like:
0. LMS7002_0 (hasSPI, hasRFchannels)

1. LMS7002_1 (hasSPI, hasRFchannels)
2. Si5351C (hasSPI)
3. FPGA (isProgramable)
4. MCU (isProgramable)
5. ....

This would allow GUI to abstract on board devices into just to single id which would be supplied to SPI, programming or other data transfering functions as destination parameter. Also listing all ICs by names would allow to connect existing GUI control panels to their respective device Ids just by matching names.

Sending/Transmitting samples:
I think the multiple channels samples should be passed as a 2D array, because most of the time all channels data are sent by interleaving them. Passing each channel with separate function call causes uncertainty when the complete samples packet could be formed, possibly missing target timestamp while not all channels are passed into packet.

from limesuite.

Made a quick additional function list for the IConnection. These should cover most of the possibly needed operations IConnection.txt, it still needs refining.
I think there should be listing of not just RFIC, but all the IC that can be controlled. Like:

I see the ICInfo, and I created a similar RFICInfo. So I will combine these two structures together to ideally, query all capabilities about a set of related hardware. I think it will make sense to expand this struct in the future if a new "technical" feature or difference arises in a future hardware.

I think maybe if there was infinite time, I would abstract out the Si5351C as just a function to set/get the external reference clock -- that some devices would implement with the Si5351C driver. Perhaps for now, we just keep it as-is.

Sending/Transmitting samples:
I think the multiple channels samples should be passed as a 2D array, because most of the time all channels data are sent by interleaving them. Passing each channel with separate function call causes uncertainty when the complete samples packet could be formed, possibly missing target timestamp while not all channels are passed into packet.

Thats a good point. I think instead of a channel index, the stream calls will take a streamId. A board with two LMS7 RFIC may have four streamids for rx0, rx1, tx0, tx1

And yes, the stream calls should take an array/vector of pointers. That works much better. Thats how SoapySDR does it as well.

from limesuite.

@rjonaitis I saw the IConnectionIntegration branch. Thats exactly what I am thinking. At this point I am trying not to break the ability to compile so I can work on the other stuff (soapy sdr and stream). But I think it should be OK to merge connection_develop into IConnectionIntegration. But when everything compiles we should merge it all into one branch.

A few new changes to IConnection and LMS64CProtocol:

• I added a general I2C API. But specifically its for the Si5351C, possibly other ICs later.
  • WriteI2C takes an address and array of bytes (string)
  • ReadI2C takes an address, number of bytes to read, yields an array of bytes (string)
• The device info has the rfic spi slave numbers (an array)
  • Also Si5351 I2C address
  • And ADF4002 slave address
• The LMS64C protocol implements SPI and I2C and dispatches to the appropriate function based on the address number

So hopefully, if things are working as planned, the class Si5351C should be able to use the I2C API now, and using the address provided by device info. :-)

from limesuite.

Looks great. I replaced LMScomms to IConnection in most modules, left out some that are not necessary for Stream board, they can be done later. Now that the IConnection pointer is going to change for each device, the pointer has to be passed into all modules upon each time the device is changed, instead of passing it to constructors as it is now.

from limesuite.

the pointer has to be passed into all modules upon each time the device is changed, instead of passing it to constructors as it is now.

Thats probably the cleanest way to do it. In this case, modules can have null IConnections, they will have to check if IConnection != null before IConnection IsOpen.

However, there is also a lazy solution using something like the connection manager. If you saw what I did to lmsComms, its basically a connection that hold a connection pointer. It implements the virtual methods and forwards them to the internal pointer.

My lmsComms changes are a temporary hack, but we can do the same basic idea for IConnection (IConnectionWrapper). This can prevent the gui classes from needing a new call to Initialize()...

from limesuite.

I saw, your current changes, that would be the easiest solution, but having entire class just for wrapping single pointer and not doing anything else seams pointless :D.
I was thinking maybe just pass pointer to a pointer of IConnection, e.g LMS7002M(IConnection **port), but that could get confusing.

from limesuite.

I was thinking maybe just pass pointer to a pointer of IConnection, e.g LMS7002M(IConnection **port), but that could get confusing.

Another option:

The GUI classes could hold a reference to a shared_ptr. We would have to check if the connection held a pointer, but the API use of IConnection would not change. Its important that a reference to a shared_pointer is held, because we still need to reset(new IConnection) in one place, in the connection dialog.

A similar option:

Similar to the shared pointer, but without the reference stuff. IConnectionWrapper would be stored as regular pointer in the wxgui classes, but it would act like a shared pointer using operator->() to get access to the internal IConnection.

class IConnectionWrapper
{
public:
IConnection *GetConnection();
void SetConnection(IConnection *);
IConnection* operator->() const;

private:
IConnection *internal;
};

Both of these options prevent us from making a useless IConnection class that overloads all of the operators.

from limesuite.

Now the wrapper options seams more convenient. It would also allow transfering of callback function pointers when changing connected device.

from limesuite.

I made IConnectionProxy: 599def7

from limesuite.

I tried using the IConnectionProxy, but the resulting code turned out differently than I expected.
It introduces additional checking of pointer to IConnectionProxy, then checking the internal pointer validity, and only then it is safe to use the -> operator to call the desired function.

LMS7002(IConnectionProxy* ptrProxy)
{
    if( ptrProxy != nullptr && ptrProxy->get() != nullptr && (*ptrProxy)->IsOpen() )
    //do stuff
}

I think best solution now would be to move the IConnection initialization for each module from it's constructor to a dedicated function module->SetConnection(IConnection*). Because the actual IConnection is changed in the top most level of the application, it can easily update all modules connections, plus that would give option to allow modules to do initializations if necessary with the newly connected device.

from limesuite.

I think best solution now would be to move the IConnection initialization for each module from it's constructor to a dedicated function

In general, I agree. I think this is the better idea. I thought I was saving some work. We either use ProxyPtr in each class, or change the top level to call Initialize() again. Anyway, just for reference. This is what I had in mind. I also put this code on the LMS7002 class as an example (we can revert it if this is a bad idea).

So, after some rewrite to make the proxy class templated so we can forward declare IConnection, it needed some additional changes. The idea was that IConnection* would be replaced by ProxyPtr in the various control classes. That is replacing the pointer with this object. And in addition, I made ProxyPtr constructable from a pointer to a pointer (IConnection * *), so for the GUI, we can change this IConnection pointer without re-initialization. Usage would look like:

LMS7002(ProxyPtr<IConnection> conn)
{
    if( conn && conn->IsOpen() )
    //do stuff
}

And in the case of the wx gui, the initialize calls would not pass IConnection *, but instead IConnection * *, for the dialogue to swap pointers out. Its not pretty. So If you have a good idea where to put the Initialize() code so its called again after dialog changes the connection, thats probably a nicer way to handle it. :-)

BTW, I merged my branch into IConnectionIntegration and got the code compiling working. But It wont work until we handle this IConnection pointer work.

from limesuite.

LMS7002(ProxyPtr<IConnection> conn)
Shouldn't proxy be passed by reference? Because making a copy would defeat the whole purpose of it.

from limesuite.

Well, it shouldnt matter. The pointers inside the ProxyPtr object get copied when its assigned. So same pointers, either IConnection * (for regular use) or IConnection ** (for this gui use case).

We can change this to LMS7002(ProxyPtr<IConnection> &conn), but there will still be an assignment inside LMS7002 when the conn is set to the internal ProxyPtr. Its really the same rules as shared_pointer, except that the internal pointer is not owned, and we have this pointer * * option as well.

Does that make sense? I know its kind of hack-ish. But it should work in principal.

To summarize this is the magic part that makes this hack work

//two different possible pointers
private:
    T **_internalRef;
    T *_internal;


//construct from ptr
template <typename T>
ProxyPtr<T>::ProxyPtr(T *ptr):
    _internalRef(nullptr),
    _internal(ptr)
{
    return;
}

//construct from ptr to pointer (gui uses this)
template <typename T>
ProxyPtr<T>::ProxyPtr(T **ptr):
    _internalRef(ptr),
    _internal(nullptr)
{
    return;
}

//the the pointer that is actually to be used
template <typename T>
T *ProxyPtr<T>::get(void)
{
    if (_internalRef != nullptr) return *_internalRef;
    return _internal;
}

from limesuite.

I see that now. Somehow we're overcomplicating this. Lets do this without the proxy, then main app is responsible for creating the connection and so it should be responsible for updating modules with new connections. This would be the most straightforward way.

from limesuite.

Yes, agreed :-)

from limesuite.

Quick update: I deleted the lmscomms, and all of the remaining classes take IConnection * for initialize. Internally, these classes dynamic cast the iconnection to the LMS64CProtocol. So these classes should work like this, and we can gradually convert things over.

And because dynamic cast could result in null if the IConnection is not actually a LMS64CProtocol. So there may be a few more places to check for m_ser != nullptr.

from limesuite.

I took care of all or most of the remaining "TODO use devIndex" code: e90a145

I know the streaming abstraction needs work, but currently the old API should be still functioning. So what else is remaining as an open issue? In other words, what else do we need to do so that all previous functionality with the GUI works again, 100%.

from limesuite.

Second update: I totally forgot about the CMD_BRDSPI_RD/CMD_BRDSPI_WR. Many of those calls above were for the SPI registers in the FPGA.

A few thoughts about FPGA registers:

• I'm not sure there is really a write flag bit (1<<31) like in the RFIC. We can forgo the 0x7fff masks and | (1<<31)
• Perhaps this merits a generic FPGA register API. Read/write 32-bit registers with 32-bit addresses. It just so happens that the the interface is SPI over LMS64C for some FPGA boards.

from limesuite.

So what else is remaining as an open issue? In other words, what else do we need to do so that all previous functionality with the GUI works again, 100%.

Mainly the MCU programming and Custom board parameters are left to do. I'm working on them right now.

Also there should to be a way of selecting which device is connected from the connectionRegistry by having actual connection pointer. Because now when the connection dialog closes, the connection index is lost, and the actual index might change if devices list changes.

I'm not sure there is really a write flag bit (1<<31) like in the RFIC. We can forgo the 0x7fff masks and | (1<<31)

Currently i don't know how FPGA handles communication direction. The LMS64C protocol idea was that the direction would be infered from command byte, and the firmware would handle the necessary bit changes, just like for the LMS7002 SPI. I'll check up how it actually works.

Perhaps this merits a generic FPGA register API. Read/write 32-bit registers with 32-bit addresses. It just so happens that the the interface is SPI over LMS64C for some FPGA boards.

I guess we could add FPGA read/write to IConnection instead of reusing transactSPI.

from limesuite.

I just wanted to ask. I see a lot of classes that have a separate Iconnection for control and data. It looks like this causes a tremendous amount of code duplication. What was the design decision behind this? Like using Stream board on USB3, and EVB7 over the ttyUSB serial connection? If thats the case, perhaps we could make a new ConnectionEntry that combines the two connections intelligently to appear as one interface... Not sure though, Thanks.

Mainly the MCU programming and Custom board parameters are left to do. I'm working on them right now.

Cool

Also there should to be a way of selecting which device is connected from the connectionRegistry by having actual connection pointer. Because now when the connection dialog closes, the connection index is lost, and the actual index might change if devices list changes.

I see the problem. One idea: We could store the ConnectionHandle for the open connection. When the dialogue is re-loaded, we select the index based on currentHandle == handles[i].

• So, for starters, I will add an equality operator for ConnectionHandle.
• And maybe, if the connection object automatically stored its ConnectionHandle used for creation, that would be helpful for solving the problem, and potentially others similar uses.

Currently i don't know how FPGA handles communication direction. The LMS64C protocol idea was that the direction would be infered from command byte, and the firmware would handle the necessary bit changes, just like for the LMS7002 SPI. I'll check up how it actually works.

I didn't see the (1<<31) in the master branch. So I think BRDSPI read/write is just inferred from the packet command enum.

Also for interest, I think that the LMS64C does not need (1<<31) for the RFIC spi, its probably added by the FX3 based on the packet command enum. However, the LMS7002M driver must set this bit anyway so that other IConnection implementations will have the correct SPI bits; especially if they just operate as a pass-through to something like spi-dev.

I guess we could add FPGA read/write to IConnection instead of reusing transactSPI.

The philosophy behind the transactSPI/and read/writeI2C was that a specific device would 1) have an PI interface, and 2) have a translation between API and physical interface in one place only. Examples are Si5351C, lms7002m, ADF4002.

However, for the FPGA registers, quite a few files need this interface. So I think its simpler to have a register interface API to keep the code simple. Even if the usage is very "device specific", having an easy API to read/write registers seems like a good idea and should extend well to other boards.

I will take a shot at this...

from limesuite.

Update:

• added read/write registers api: 4e19710
• added connection dialog index: 9fc5c65

from limesuite.

I just wanted to ask. I see a lot of classes that have a separate Iconnection for control and data. It looks like this causes a tremendous amount of code duplication. What was the design decision behind this? Like using Stream board on USB3, and EVB7 over the ttyUSB serial connection? If thats the case, perhaps we could make a new ConnectionEntry that combines the two connections intelligently to appear as one interface... Not sure though, Thanks.

At first there was only the control connection, the separate data connection had to be done when Stream and EVB7 was needed to be connected simultaneously through COM and USB connections. The problem with creating a single combined connection of these, is that it's impossible to detect which ports should be paired. As there can be more devices connected to PC that appear as COM ports, the ammount of possible combinations of them and USB devices grow exponentialy. Also the same Stream & EVB7 combo can be connected with just the Stream USB port, depending on some resistors on the board, that adds even more possible connection options. So passing two connections for control and data was more flexibile and simplier than trying to combine them.

Also for interest, I think that the LMS64C does not need (1<<31) for the RFIC spi, its probably added by the FX3 based on the packet command enum. However, the LMS7002M driver must set this bit anyway so that other IConnection implementations will have the correct SPI bits; especially if they just operate as a pass-through to something like spi-dev.

Yes the LMS64C ignores the (1<<31) bit, and sets it according to command enum. Yeah the Novena communication required that bit set when working with spi-dev, at that time i was adding it inside LMScomms instead of LMS7002 logic.

from limesuite.

So passing two connections for control and data was more flexibile and simplier than trying to combine them.

I see what you mean.

It should be possible to expand the STREAM connection to discover COM devices and return USB + COM connection handle results. And then the STREAM Connection can re-use the COM connection code for the SPI (other several other) interfaces. So rather than two separate device columns in the dialog, you would get only one:

• STREAM0 USB
• STREAM0 USB + /dev/ttyACM1
• STREAM0 USB + /dev/ttyACM2
• STREAM1 USB
• STREAM1 USB + /dev/ttyACM1
• STREAM1 USB + /dev/ttyACM2

This would simplify the code in many places, but yielding all of the connection discovery results is sort of an N x M problem, for N stream boards, and M COM connections. But then maybe some of these results could be eliminated by performing test communications. And it would only look bad in extreme cases with too many COM ports. Anyway, its just a thought.

from limesuite.

But then maybe some of these results could be eliminated by performing test communications. And it would only look bad in extreme cases with too many COM ports. Anyway, its just a thought.

I was thinking about doing that, but performing communications test to actually know what device is connected to each COM port can add up to great amount of time, considering that most of them would not respond to query and would need to wait for timeouts.

This might not be a case on Linux because not many devices appear as ttyACM. But for Windows users (at least for other developers that have many devices pluged in that appears as COM) this can be really annoying.

from limesuite.