Check for bus number 0 in spiSetBusInstance() #11088
Conversation
SteveCEvans
force-pushed
the
io_def_owner
branch
from
e8ce157
to
212e21c
Compare
|
|
src/main/drivers/io.c
Outdated
| @@ -343,7 +343,7 @@ void IOConfigGPIO(IO_t io, ioConfig_t cfg) | |||
|
|
|||
| void IOConfigGPIOAF(IO_t io, ioConfig_t cfg, uint8_t af) | |||
| { | |||
| if (!io) { | |||
| if ((!io) || (IO_GPIOPortIdx(io) < 0) || ((unsigned int)IO_GPIOPortIdx(io) >= ARRAYLEN(ioPortDefs))) { | |||
There was a problem hiding this comment.
The original idea was that you can't get invalid io - macros + ioTag mapping tried to ensure that.
It may be reasonable to be defensive here, but invalid io error shall be propagated somehow (other functions may fail too and validity check will cause considerable overhead). Also all targets shall implement similar handling.
Another possibility to to use macros / static assert to check that ioPortDefs is defined for all enabled pins / nonzero ioDefUsedMask entries.
There was a problem hiding this comment.
Please hold fire on this PR. I'm not happy to just be catching a bad param being passed. I'm tracking down where that comes from.
SteveCEvans
dismissed stale reviews from haslinghuis and DusKing1
via
71f8eb8
SteveCEvans
force-pushed
the
io_def_owner
branch
from
212e21c
to
71f8eb8
Compare
SteveCEvans
changed the title
|
I've found and fixed the underlying issue which was causing memory corruption |
| @@ -486,7 +486,7 @@ static void spiRxIrqHandler(dmaChannelDescriptor_t* descriptor) | |||
| // Mark this bus as being SPI | |||
| bool spiSetBusInstance(extDevice_t *dev, uint32_t device) | |||
| { | |||
| if (device > SPIDEV_COUNT) { | |||
| if ((device == 0) || (device > SPIDEV_COUNT)) { | |||
There was a problem hiding this comment.
What about
#define SPI_CFG_IS_VALID(device_cfg) (SPI_CFG_TO_DEV(device_cfg) >= 0 && SPI_CFG_TO_DEV(device_cfg) < SPIDEV_COUNT)
Also, consider refactoring uint32_t device to uint32_t device_cfg. Or even better, rename _cfg to _tag everywhere( IIRC _cfg is used as configuration struct in other parts of code).
This mixed zero and one based indexing will eventually lead to some headaches ..
There was a problem hiding this comment.
@SteveCEvans please take a look at the suggestion.
There was a problem hiding this comment.
There are other cases where we have 0 based and 1 based numbering. Always a source of confusion. For example pinioSet(0, 1) sets PINIO 1.
I’d rather look at this in another PR, post 4.3.
There was a problem hiding this comment.
Reason for 1 based indexing (when implementing iotag) was to allow 'no pin' when memory is initialized to zeroes.
But C type checking makes it difficult to separate IO from io_tag, it was partially solved by using pointer type for IO.
It may be possible to use structs to force type checking, but it is quite hard to hide all implementation detail completely.
Fixes: #11087