uavcan_stm32: add apis to support ardupilot way of using uavcan #130
Conversation
|
Thanks, Tom. As I said in the chat room, can we please find some way of reducing the amount of duplicated code? |
…rate can iFace init
|
well, I've reduced it so init() just loops through init(i) and it takes less LOC y only doing the actual work in a single function but the compiled output on ArduPilot is 16 bytes larger. So, it seems the compiler is better at optimizing than me/humans. However, its easier to maintain. Let's not merge yet. I'd like to test a little more |
|
I'll test on ArduPilot, could someone else please test on other platforms? |
|
|
||
| UAVCAN_STM32_LOG("Initing iface %i...", can_number); | ||
|
|
||
| if (can_number == 0) { |
There was a problem hiding this comment.
Opening brace goes on the next line.
| ifaces[1] = &if1_; // Same thing here. | ||
| res = if1_.init(bitrate, mode); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Per MISRA, every condition that contains an else-if should have a plain else clause at the end. In this case, it must handle the case when can_number is neither 0 nor 1. There are several other occurrences where this should be updated.
There was a problem hiding this comment.
how about a switch statement with an empty default?
There was a problem hiding this comment.
If you find that reasonable, sure (in my opinion if works well here)
| UAVCAN_STM32_LOG("Iface 1 init failed %i", res); | ||
| ifaces[1] = UAVCAN_NULLPTR; | ||
| res = configureIface(bitrate, mode, can_number); | ||
| if (res < 0) { |
|
all braces fixed and I've added error handling with an else in a few places |
|
Looking at the number of changes and considering the fact that they are temporary, I am beginning to question whether it all makes sense. Would it not be better to just fork the driver immediately and maintain your own copy? |
|
it's been forked for a year. The change was small until you wanted it optimized. Optimizing it made it messy and made the program size larger. |
|
You are right, I should have foreseen this earlier, my fault. I'm just saying that the ArduPilot project might benefit from forking the driver completely and removing all of the unnecessary stuff from it (like FreeRTOS and baremetal support). I am going to remove all of the hardware-specific stuff from this repo in the future anyway. Do you still want this PR merged? |
|
I brought this up during the weekly ArduPilot Dev Call. We decided that we should merge this in (after more testing) and then we'll update the ArduPilot fork to match this upstream repo. Please hold the merge until we can test it a little more |
|
Closing because the drivers are being moved out of the repo (#143); if still relevant, please submit this changeset to the new repo at https://github.com/UAVCAN/libuavcan_stm32. |
|
But I would suggest you instead to just create your own driver with your preferred API. |
uavcan_stm32: add apis to support ardupilot way of using uavcan
These changes are to support ArduPilot hardware infrastructure differences over multiple embedded OSs including Nuttx, ChibiOS, BareMetal, and FreeRTOS implementations.