[driver] Adding Semtech SX1280/SX1281 driver

[rasmuskleist]

Thanks @LasseJensen213 for contributing to this!

Before finishing up this driver I have a couple of points that I would like your opinion on:

1. Similar to this implementation, I have added a hardware abstraction layer. This layer should allow using both spi and uart (yet to be implemented) similar to the transport layer in e.g. lis3. This layer also handles the differences between the command structure of the spi and uart interface, for example that the uart interface should also send the number of params. Furthermore, the register access and data buffer operations have different structures, so these functions have also been implmented in hal. Because this layer handles more than just mere transport I choose to call it hal. Currently I am not satisfied with the implmentation for the following reason:

   • I need to implement the uart hal, but in the current implemtation of modm the uart interface is somewhat different from both spi and i2c. In addition I have not been able to find a driver in modm, which uses uart. Is there any good reason why there is no resumable and device implementation of uart? If so how do you envision a driver using uart should be implemented?
   • I would like to have the sx128x struct members available in hal. However, if I simply let both Sx128x and Sx128xHalSpi inherit from sx128x I get the warning -Winaccessible-base and several compile errors e.g. 'modm::sx128x' is an ambiguous base. Meanwhile, if only Sx128xHalSpi inherit from sx128x then the members of sx128x are not available to Sx128x. Do you have any work around to this?
   • I am debating if I should simply have a transport layer which implements writeCommand and readCommand. These take a struct opcode and optionally offset or address. In addition it should take an array containing the parameters. These functions then figure out depending on the opcode if any of the optional parameters should be printed. What do you think about this?

2. Currently the definitions contains many duplicates in different name spaces. The idea behind this implementation is to make it very clear what definitions are available to each different packet engine. However, I think this implementation has two big problems. First of all it involves many nested namespaces so accessing members becomes very verbose. Second of all it makes it more difficult to maintain with code duplication. Two other options are:

   • The enum classes can be in the namespace sx128x and joined. With this implmenetation some options may not be applicable and a number may have different meanings. For example the bitrates are different between GFSK and FLRC and the value 0x45 means Br1000Bw1200 for GFSK and Br1300Bw1200 for FLRC. Hence only a subset of the resulting bitrate enum class would be applicable to GFSK and FLRC, respetively. I could specifiy which are applicable with comments.
   • The enum classes can be in the namespace sx128x and named according to which packet engine it applies to. This is implemented here. For example the bitrates are shared between GFSK and BLE, menawhile it is different for FLRC. The set of bitrates applicable to GFSK and BLE would then be called something like BitrateGfskBle and BitrateFlrc, respectively.

Thanks in advance for your comments, I really appreciate it! I should add that I have tested that I can send and recieve packets with LoRa.

[salkinium]

(same thing with the &arg-> *arg conversion here)

Because this layer handles more than just mere transport I choose to call it hal

Please call it anything else but HAL, it has a predefined meaning in embedded software and it's not this. Maybe Protocol Layer or something.

Is there any good reason why there is no resumable and device implementation of uart? If so how do you envision a driver using uart should be implemented?

UART has no state like SPI CS or I2C Transactions, it can only send packets. The protocol on top needs to be implemented by the caller. The async interface is basically RF_WAIT_UNTIL(Uart::write(byte)). Note that the buffers are off by default, you need to enabled them via an lbuild option. You can static assert their minimal size in your protocol driver though.

I have not been able to find a driver in modm, which uses uart.

That's because there are few drivers that use UART due to requiring a higher level framing, which is annoying to implement in an ASIC without CPU. I2C/SPI are much simpler in that respect.
You can, however, look at the very complicated AMNB protocol driver, which uses UART very extensively.

if I simply let both Sx128x and Sx128xHalSpi inherit from sx128x I get the warning -Winaccessible-base and several compile errors e.g. 'modm::sx128x' is an ambiguous base.

The inheritance is only a convenience for the developer.
I would only inherit modm::Sx128x from modm::sx128x and use the full qualifiers or a typedef for Sx128xHalSpi.

I should simply have a transport layer which implements writeCommand and readCommand.

Sure, whatever is the minimal set of function that need to be ported between SPI and UART. It's not an exposed API to the end user so it doesn't need to have all the syntax sugar of MODM_FLAGS for example.

Currently the definitions contains many duplicates in different name spaces.

Yes it's annoying to write and maintain, but the other options are not necessarily easier to maintain either. I think its fine with the union as it is now.

[rasmuskleist]

Thanks for the nice comments!

I have changed references to pointers. I will try to look into the AMNB protocol driver.

Also I have been debating if I should make the transport layer much light so it is mere transport. Specifically, I was considering if I should declare a Sx128xTransport which defines the interface and a Command struct which contains the opcode, and vargs. The vargs would be empty for all opcodes expect register access and buffer operation commands. Thereby I can make the transport layer first write the contents of the Command struct followed by a waitCycle for the case of SPI read or the length for the case of UART read/write. I believe this will also make it possible to handle special cases such as getStatus, which is the only read operation that that does not have a waitCycle with SPI. There may be smarter ways to go about this, or what do you think?

[rasmuskleist]

I should have addressed most issues by now I believe. What is now missing are the following

• Implement (non-blocking) reset function
• Fully implement UART transport layer
• Implement and streamline logic in driver functions

With regards to the reset function I am debating if the driver should take a third template argument, the reset pin, or if the reset function should take the reset pin as a template argument? At the moment I cannot imagine calling the reset function anywhere inside the driver.

For the UART transport layer, I am considering if it makes sense to add UartDevice to architecture? The DeviceWrapper in AMNB seem to be very generic and something that can be reused for example in the sx128x driver. What are your thoughts on this?

Finally, I am debating how much logic should be implemented in the driver to ensure correct usage. After all it is a fairly complicated driver, so covering every possible inccorrect use case may be quite involved. At the moment I have kept the driver fairly light weight and have put the responsiblity of correct usage onto the user. In fact the only logic that I have implemented so far is to clear the interrupts before setting TX and RX. However, I think it may be better to add a warning that the interrupts should be cleared before setting TX or RX and leave it up to the user. This is because I imagine every use case to manually clear the interrupt in the protothread as in the example. What is your viewpoint on this?

[salkinium]

if the driver should take a third template argument

Probably yes, cos that's how all other drivers do it.

I am considering if it makes sense to add UartDevice to architecture?

You mean a wrapper that provides generic transaction-like semantics?

1. write
2. wait for write finish
3. read
4. wait for read finish

Yeah, I can see that being a useful pattern for a bunch of UART-based drivers.

I am debating how much logic should be implemented in the driver to ensure correct usage. After all it is a fairly complicated driver, so covering every possible inccorrect use case may be quite involved

It's probably best to first understand the typical usage and then apply the error detection/recovery for those paths first.
Otherwise you're perhaps implementing something that isn't even how people (ab)use it in the real world.
The driver doesn't need to be perfect, you can always come back and fix it later if you find that some things annoy you.

[rasmuskleist]

I should have now fully implemented the UART transport layer. What is now missing is to handle some special cases where the read value from the radio should be manipulated e.g. the rssi. Typically this is done in for example a Data class in modm, which is nice because it keeps the driver lean. However, for the PacketStatus struct this would mean having to implement duplicate getter method for each PacketType, a lot of which are duplicates. Therefore it may be advantageous to implement this conversion inside the driver instead. Especially to keep things consistent because the getRssi function in the driver should do the conversion as well. What are your thoughts on this?

[salkinium]

Very thorough driver, almost done, right?

What are your thoughts on this?

You can remove the data class completely if you want. It's a bad convention anyways. Then you don't need to duplicate configuration data everywhere.

(There appears to have been some mishap with the rebase, there are commits in here that are already in develop.)

[rasmuskleist]

Yes I believe its done, at least for now. As you write it is very difficult to predict how people (including myself) will (ab)use the driver at this point. Therefore I would rather do some changes at later to make it easier to use. I presume that I can do some breaking changes at later with this driver if necessary? I also decided to dumb the driver down, so the driver only facilitates a nice interface for issuing the commands. This is for simplicity and consistency.

I will confirm that the changes made to the driver physically works tonight and convert to PR. Also I should add that I discovered the EXTI bug to be resolved with #1052, when testing this driver. It caused a lot of confusion so I am happy to see that there is already a fix for it!

I don't know how I messed that rebase up, but it should be fixed now.

[chris-durand]

If you need the EXTI fixes from #1052, you can cherry-pick the commits and add them to this PR. I probably won't find any time at work this week to finish my PR.

I'll have a look at this driver tomorrow.

[chris-durand]

Thank you!

[rasmuskleist]

Thank you for your comments! I should have addressed your comments now. With regards to your fix I can easily wait until your PR is ready. At the moment I will just use the device as if there was a single interrupt pin by issuing getIrq.

[salkinium]

Thanks for another great driver!

[salkinium]

(It would be good if you could resolve the conversations yourself, when you feel like you addressed them. Then we don't have to check again and it's clear to us what the state is. We can always unresolve them if something comes up.)

[rasmuskleist]

Thanks! Yes I will do that going on.