refactor: sensors abstraction - #4
Conversation
bb87256 to
5f87fa0
Compare
5f87fa0 to
d437135
Compare
|
Don't merge yet, just putting it here for early review |
|
Looks good, assuming you just need to test this? Also, I feel like the "handle_as" functions would be better named "handle_get_as" |
|
I guess I am just not 100% convinced yet that we need this level of abstraction for our sensors? Do we get a lot out of it in terms of maintainability and ease of understanding code? I wonder if it would be easier to just make specific implementations for each sensor, and let it be up to the device to determine how they want to use all that. |
| } uart; | ||
| struct handle_uart { UART_HandleTypeDef *handle; }; | ||
|
|
||
| #define HANDLE_QSPI(handle) \ |
There was a problem hiding this comment.
Good catch, should be handle QSPI
| #define HANDLE_SPI(in_handle, in_port, in_pin) \ | ||
| (struct handle) { \ | ||
| .protocol = SPI, \ | ||
| .serial = { \ | ||
| .spi = { \ | ||
| .pin = (in_pin), \ | ||
| .port = (in_port), \ | ||
| .handle = (in_handle), \ | ||
| } \ | ||
| } \ | ||
| }; | ||
| #else | ||
| struct handle_spi { int placeholder; }; | ||
| #define HANDLE_SPI(handle, port, pin) assert(0 & "SPI is not enabled!"); | ||
| #endif |
There was a problem hiding this comment.
Not sure if I really love this HANDLE_SPI macro stuff. I think I see where you're coming from with having the stub implementation that would fail if someone in the device if struct handle_spi x = HANDLE_SPI(y,z,a), but unless a spi is explicitly configured from the .ioc file, there wouldn't be a well defined handle, or cs pin to add, to setup anyways? I think we are covering a case that is essentially impossible to run into.
There was a problem hiding this comment.
I am still back and forth with the macro, you are right that compilation will not be allowed if we have an illdefined handle, but at the same time there is no error message shown indicating why. With the macro you get a clear message on why. I am willing to drop this if we think that is not an issue.
There was a problem hiding this comment.
I don't really think it will be an issue because you would not be able to put the periphal as the handle like you would be expected to. If someone tried to do struct handle_spi bmi088 = { .... , .handle = &hspi1} there would be several failures, one of them being that there isn't even an spi1 to be a handle in the first place.
There was a problem hiding this comment.
Yes, that is true. But the reason why you know that the compilation failed is due to missing modules is because you have worked on this before. For a new member they might assume that all modules are pulled in, or they might question why there isn't a SPI handle definition. That is not to say I am opposed to removing the macros.
| //// To create a handle, you MUST use the following methods | ||
| //// struct handle spi = HANDLE_SPI(&hspi1, port, pin); | ||
| //// struct handle i2c = HANDLE_I2C(&hi2c1, address); | ||
| //// struct handle uart = HANDLE_UART(&huart1); | ||
| //// struct handle qspi = HANDLE_QSPI(&hqspi); | ||
| //// Then you may pass the handle to the initialization function of a device. |
There was a problem hiding this comment.
I am not certain that this abstraction is needed.
There was a problem hiding this comment.
See previous comment.
| //// | Serial API Abstractions | | ||
| //// All serial protocols implement an API with the same write and read | ||
| //// functions, with differing implmentations of course. The serial api contains | ||
| //// the handle and is requested by a device in its initialization function. | ||
| //// | ||
| //// See the currently provided drivers for an example. But the gist is | ||
| //// int8_t sensor_init(struct sensor_ctx *ctx, struct sensor *sensor, struct handle *handle) | ||
| //// { | ||
| //// assert(handle->protocol == SPI); | ||
| //// sensor->ctx = ctx; | ||
| //// sensor->read = sensor_read; | ||
| //// serial_api_spi(ctx->api, handle); | ||
| //// } | ||
| //// Of course you may decide to support multiple protocols if you wish, which | ||
| //// this abstraction makes really simple. You would pass `api` around and call | ||
| //// `api->write(...)` and `api->read(...)` for writing and reading respectively. |
There was a problem hiding this comment.
I appreciate the comments, they are very helpful (nothing to resolve I just want to say that I like them)
|
Just to be clear, I am by no means the architect of this code and you should talk to Dhruv about anything major with regard to that |
|
The main pro of abstracting them to this thin api class is that we keep all the #ifdefs in one place. The sensors don't (and shouldn't) care about what protocol is used. With previous approaches one of the big issues was how to handle
This abstraction, to some degree, resolves 1. by collecting all #ifdefs and protocol definitions in one file, and 2. by allowing to simply use the On the point of maintainability and readability, we concretely defined three actions that every sensor/device has to do
This means every implementation after can simply think of On maintainability, since this is the way every device/sensor communicates, we can have the implementation in a single place, with much more eyes on it, rather than the same implementation scattered across the codebase. Sorry for the long reply and rambling. |
3 participants
No description provided.