Skip to content
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

Fix stm32 periph headers #20531

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Enoch247
Copy link
Contributor

@Enoch247 Enoch247 commented Apr 2, 2024

Contribution description

This PR should not change what is built. It just moves some things out of shared headers into peripheral specific headers that already existed.

Testing procedure

I checked that it still builds by running make -C ../../tests/periph/uart BOARD=nucleo-f767zi. I could not find a similar test for usbdev.

Issues/PRs references

none known

Enoch247 added 2 commits April 2, 2024 12:27
This patch moves the SPI related defines and declarations out of the
shared stm32 periph header into the SPI specific one.
This patch moves the USB related defines and declarations out of the
shared stm32 periph header into the USB specific one.
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Apr 2, 2024
@@ -39,6 +39,65 @@ extern "C" {
*/
#define USBDEV_CPU_DMA_REQUIREMENTS __attribute__((aligned(USBDEV_CPU_DMA_ALIGNMENT)))

#if !DOXYGEN /* hide implementation details */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I have moved this to cpu/stm32/include/usbdev_stm32.h instead?

#endif

/**
* @name Override the SPI mode values
Copy link
Contributor Author

@Enoch247 Enoch247 Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming if I do the following it will make CI/CD happy (based on the HAVE_SPI_CLK_T below). Is this the right thing to do? Or should I document the struct and macro separately?

Suggested change
* @name Override the SPI mode values
* @brief Override the SPI mode values

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can test if the CI is happy with a change by running the dist/tools/doccheck locally

reading
https://www.doxygen.nl/manual/grouping.html#memgroup
https://www.doxygen.nl/manual/commands.html#cmdname

i think it would be better to have it like i will write in the suggesting comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the links. I setup to build the docs locally and am now playing around with your suggestion. I am seeing some weird behavior. According to those links, member groups may not be nested. The entire file is in a groups. This is a common idiom used through-out the RIOT source tree. This means that anywhere the member group mechanism is used, we are nesting them inside the one that wraps the entire file. Maybe this explains the weird things I am seeing?

Copy link
Contributor Author

@Enoch247 Enoch247 Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After doing more research, I'm 99% certain that is what is going on here. The file wide member group is being canceled as soon as Dozygen sees a nested group inside it. Then at the end of the nested group, Doxgen is placing all artifacts outside of the file-wide one. Essentially, Doxygen does not seem to maintain a stack for managing groups. It just enters the group it last sees and when it hits the end of a group it pops out to non-group namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #20537 to track this issue separately.

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 2, 2024
@riot-ci
Copy link

riot-ci commented Apr 2, 2024

Murdock results

✔️ PASSED

1d3e10b cpu/stm32/periph: move USB header bits to proper place

Success Failures Total Runtime
10027 0 10027 10m:28s

Artifacts

Comment on lines +98 to +113
/**
* @name Override the SPI mode values
*
* As the mode is set in bit 3 and 2 of the configuration register, we put the
* correct configuration there
* @{
*/
#define HAVE_SPI_MODE_T
typedef enum {
SPI_MODE_0 = 0, /**< CPOL=0, CPHA=0 */
SPI_MODE_1 = STM32_SPI_CPHA_Msk, /**< CPOL=0, CPHA=1 */
SPI_MODE_2 = STM32_SPI_CPOL_Msk, /**< CPOL=1, CPHA=0 */
SPI_MODE_3 = STM32_SPI_CPOL_Msk | STM32_SPI_CPHA_Msk, /**< CPOL=1, CPHA=0 */
} spi_mode_t;
/** @} */

Copy link
Contributor

@kfessel kfessel Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* @name Override the SPI mode values
*
* As the mode is set in bit 3 and 2 of the configuration register, we put the
* correct configuration there
* @{
*/
#define HAVE_SPI_MODE_T
typedef enum {
SPI_MODE_0 = 0, /**< CPOL=0, CPHA=0 */
SPI_MODE_1 = STM32_SPI_CPHA_Msk, /**< CPOL=0, CPHA=1 */
SPI_MODE_2 = STM32_SPI_CPOL_Msk, /**< CPOL=1, CPHA=0 */
SPI_MODE_3 = STM32_SPI_CPOL_Msk | STM32_SPI_CPHA_Msk, /**< CPOL=1, CPHA=0 */
} spi_mode_t;
/** @} */
/**
* @{
* @brief Override the SPI mode values
*
* As the mode is set in bit 3 and 2 of the configuration register, we put the
* correct configuration there
*
*/
#define HAVE_SPI_MODE_T
typedef enum {
SPI_MODE_0 = 0, /**< CPOL=0, CPHA=0 */
SPI_MODE_1 = STM32_SPI_CPHA_Msk, /**< CPOL=0, CPHA=1 */
SPI_MODE_2 = STM32_SPI_CPOL_Msk, /**< CPOL=1, CPHA=0 */
SPI_MODE_3 = STM32_SPI_CPOL_Msk | STM32_SPI_CPHA_Msk, /**< CPOL=1, CPHA=0 */
} spi_mode_t;
/** @} */

the @brief is not needed (the first sentence is the brief-desciption if no @brief exists

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants