-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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: Refactor MCP23009 GPIO expander into uORB driver #21466
Conversation
8bcb9d0
to
e187328
Compare
e187328
to
bb6f29e
Compare
bb6f29e
to
7a1f0af
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.
Changed the read method to async polling via callback.
@@ -250,8 +250,8 @@ | |||
#define GPIO_VDD_3V3_SD_CARD_EN /* PC13 */ (GPIO_OUTPUT|GPIO_PUSHPULL|GPIO_SPEED_2MHz|GPIO_OUTPUT_CLEAR|GPIO_PORTC|GPIO_PIN13) | |||
|
|||
/* MCP23009 GPIO expander */ | |||
#define BOARD_GPIO_VDD_5V_COMP_VALID "/dev/gpin4" | |||
#define BOARD_GPIO_VDD_5V_CAN1_GPS1_VALID "/dev/gpin5" |
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.
Well, well, well… I was wondering why my driver worked in firmware_private, but not in upstream.
Turns out, the pins are actually registered at /dev/gpio4
and /dev/gpio5
. So the function ADC::open_gpio_devices()
always got -1
as file handles and thus never actually called the MCP23009 driver.
This was working when the driver was added (e2337a3), but since then NuttX changed the format string to generic gpio
.
So since then, apparently nobody ever noticed this stopped working… 🫠
Fortunately these are the only occurances of /dev/gpin
, /dev/gpint
, and /dev/gpout
in PX4.
We should be careful when we upgrade NuttX in firmware_private.
fi | ||
if ver hwtypecmp V5X00a000 V5X00a001 V5X008000 V5X008001 | ||
then | ||
mcp23009 start -b 3 -X -D 0xf1 -O 0xf0 -P 0x0f -U 10 |
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.
Is reading the pin every 10ms too much? The ADC tasks queries this data every 10ms, but since it's async now, the data may be 20ms delayed in the worst case (probably even more with prioritized scheduling).
{ | ||
mcp23009_gpio_dev_s *gpio = (struct mcp23009_gpio_dev_s *)dev; | ||
return gpio->obj->go_write(gpio->id, value); | ||
*value = fromGpioIn.input & gpio->mask; | ||
return OK; |
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.
So the read now relies on the driver periodically reading the GPIO and notifying this interface of the mask. It's… UNBLOCKABLE, muhoha!
Set the output value on device /dev/gpin1 to high | ||
$ gpio write /dev/gpin1 1 | ||
Set the output value on device /dev/gpio1 to high | ||
$ gpio write /dev/gpio1 1 |
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.
Only a documentation fix, while I'm here.
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.
I just found 2 smaller details, otherwise looks good.
I think we can do w/o going through NuttX, but this is fine for now.
7a1f0af
to
831a791
Compare
Fixed. |
831a791
to
946017c
Compare
Solved Problem
The previous mechanism registered the MCP23009 as GPIO pins inside NuttX, so that any thread (
nsh_main
on upstream,wq:hp_default
on Skynode 2.7 release) can cause a write or read on I2C3. This is protected by a semaphore inside the NuttX I2C driver, which caused the thread to get boosted.Solution
This moves the MCP23009 into a proper driver which runs on a I2C work queue and has a uORB interface. This makes access asynchronous and prevents thread boosting.
This seems to only be an issue on the FMUv5x, the other versions don't use the MCP23009.
Alternatives
We could also simply™ fix™ NuttX's priority inheritance.
Test coverage
Verified with the debugger to ensure the functions are called correctly.
Context
Callgraph of PX4-Autopilot and the internal Skynode branch before the change, note the
nsh_main
andwq:hp_default
tasks calling into the I2C transfer function after initialization.Callgraphs after the change.