-
Notifications
You must be signed in to change notification settings - Fork 130
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
[driver] Adding Semtech SX1280/SX1281 driver #1050
Conversation
(same thing with the &arg-> *arg conversion here)
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.
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
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.
The inheritance is only a convenience for the developer.
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.
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. |
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 |
I should have addressed most issues by now I believe. What is now missing are the following
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? |
Probably yes, cos that's how all other drivers do it.
You mean a wrapper that provides generic transaction-like semantics?
Yeah, I can see that being a useful pattern for a bunch of UART-based drivers.
It's probably best to first understand the typical usage and then apply the error detection/recovery for those paths first. |
e9d0c6e
to
c6c3a31
Compare
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? |
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.
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.)
595dfbd
to
c03bd15
Compare
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. |
c03bd15
to
3c065ec
Compare
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. |
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.
Thank you!
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. |
4c9246f
to
517bd84
Compare
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.
Thanks for another great driver!
(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.) |
Thanks! Yes I will do that going on. |
Thanks @LasseJensen213 for contributing to this!
Before finishing up this driver I have a couple of points that I would like your opinion on:
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:
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:
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.