Skip to content

Conversation

@matthijskooijman
Copy link
Contributor

This looks up the SPI module corresponding to each of pins passed and
then uses pinmap_merge_peripheral() to merge this in a single pin. If
not all pins map to the same SPI module, this should abort.

The pinmap_merge_peripheral function returns NP to indicate a mismatch
between the passed peripherals. However, when one of the arguments is
already NP, this is not propagated into the function result, instead the
other argument is simplly returned.

In this case, the merging happens in two levels: First MISO and MOSI are
merged, then SCK and SS are merged and finally the merged results are
again merged. That means that if MISO and MISO mismatch, but SCK and SS
match (or the other way around), the NP returned due to the mismatch is
ignored and the final result is not NP, so no error is raised.

This changes the code to also check the intermediate merge results for
NP, to properly detect mismatches.

Note that the actual error message produced using core_debug() is disabled by default (which might need some additional work to somehow elegantly enable that), but this will at least make sure that the check in the code is correct.

This looks up the SPI module corresponding to each of pins passed and
then uses `pinmap_merge_peripheral()` to merge this in a single pin. If
not all pins map to the same SPI module, this should abort.

The `pinmap_merge_peripheral` function returns NP to indicate a mismatch
between the passed peripherals. However, when one of the arguments is
already NP, this is not propagated into the function result, instead the
other argument is simplly returned.

In this case, the merging happens in two levels: First MISO and MOSI are
merged, then SCK and SS are merged and finally the merged results are
again merged. That means that if MISO and MISO mismatch, but SCK and SS
match (or the other way around), the NP returned due to the mismatch is
ignored and the final result is not NP, so no error is raised.

This changes the code to also check the intermediate merge results for
NP, to properly detect mismatches.
@fpistm fpistm self-requested a review December 11, 2019 06:51
@fpistm fpistm added the fix 🩹 Bug fix label Dec 11, 2019
@fpistm fpistm added this to the 1.8.0🎄 🎅 milestone Dec 11, 2019
@fpistm
Copy link
Member

fpistm commented Dec 11, 2019

Hi @matthijskooijman
Nice catch.
Thanks

@fpistm fpistm merged commit 85094e0 into stm32duino:master Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix 🩹 Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants