mbed-ce / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
37 stars 12 forks source link

fix ambiguous global enum definition for SPI in OSPIFBlockDevice driver #291

Closed timIdeaTech closed 1 week ago

timIdeaTech commented 2 weeks ago

Summary of changes

OSPIFBlockDevice.h defined "SPI" as a global enum under the name _mode.

Both are collision/ambiguity hazards so, using the global enum convention of the existing driver code

enum _mode {
    SPI,
    SOPI,
    DOPI
};

has been changed to:

enum ospif_opi_mode {
    OSPIF_OPI_MODE_SPI = 0, /* SPI mode (OPI modes disabled)*/
    OSPIF_OPI_MODE_SOPI,    /* STR-OPI mode */
    OSPIF_OPI_MODE_DOPI     /* DTR-OPI mode */
};

as well as any references in OSPIFBlockDevice.cpp

Impact of changes

Any reference to the previous enum's in user code will result in preprocessor not defined errors.

Migration actions required

All references to the previous enum's in user code will have to be updated with the new prefix OSPIF_OPIMODE

Documentation

Update references in change_mode() references in https://mbed-ce.github.io/mbed-os/class_o_s_p_i_f_block_device.html)


Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@multiplemonomials

timIdeaTech commented 2 weeks ago

Not sure I follow the 2 failed checks above. I emulated the other enum comments for style and I'm not sure what the other is...

If I can fix it, let me know. If not, let me know how I can avoid this in the future.

I'm assuming I'm not on the hook for the mbed-ce html doc changes, correct?

multiplemonomials commented 2 weeks ago

For the style check it appears to be complaining about the space after the asterisk. Dang astyle it can be so finicky sometimes...

multiplemonomials commented 2 weeks ago

As for the other error, it would appear that on this line, #SPI needs to be changed to #mbed::SPI. I think that Doxygen was linking it to this enum instead of the SPI class!

timIdeaTech commented 1 week ago

Sorry, just figuring this all out. Should be good to merge the 3 files except doxygen-awesome-css. Not sure how to back out that one.