Skip to content

Conversation

@gmarull
Copy link
Member

@gmarull gmarull commented Oct 3, 2020

The script is able to generate common LL headers which can be used in
both Zephyr and applications to make it easier to create family
independent code.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

I'm globally fine with this proposal. Thanks for this addition!

2 requests, though:

  • I'd prefer headers to be moved into stm32cube/ within a "common_ll/include" folder or similar.
  • While available ll_usb.h are not a public api but are

Also, IIC following files are missing: _dma2d.h, _i2c.h

@gmarull gmarull requested a review from erwango October 5, 2020 12:32
@gmarull
Copy link
Member Author

gmarull commented Oct 5, 2020

  • While available ll_usb.h are not a public api but are

What should we do with ll_usb.h?

@erwango
Copy link
Member

erwango commented Oct 5, 2020

ll_usb

My proposal is to make a blacklist of drivers for which api are not public, and don't generate header file for these ones.
From what I'm aware of, there would be only usb for now, but it would be easy to add more if needed (or remove in case api is made public).

Copy link
Contributor

@KozhinovAlexander KozhinovAlexander left a comment

Choose a reason for hiding this comment

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

Except of some small style changes this PR looks very straight-forward and necessary to me.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

One nitpick.
Last request: We'd need a way to identify the Cube package versions that were used for this generation.
It could be "direct" by logging each cube package version that was used, that would be ideal.
It too much trouble, we nca use an "indirect" method by providing the SHA1 used for generation.

if not entry.is_dir() or not entry.name.startswith("stm32"):
continue

family = entry.name
Copy link
Member

Choose a reason for hiding this comment

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

To remain in line with usual vocabulary, please use series instead of family.
For instance: we usually refer as stm32 family and f1 series.

Copy link
Member Author

Choose a reason for hiding this comment

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

adjusted names

@gmarull
Copy link
Member Author

gmarull commented Oct 12, 2020

@erwango CubeMX is not used for the generation of these headers, the script is pointed to the stm32cube folder in the repo, not sure how can we track that.

@erwango
Copy link
Member

erwango commented Oct 13, 2020

@erwango CubeMX is not used for the generation of these headers, the script is pointed to the stm32cube folder in the repo, not sure how can we track that.

I know I was rather referring to the Cube HAL packages versions.
One idea is to force the generation after each Cube HAL update.

@gmarull
Copy link
Member Author

gmarull commented Oct 13, 2020

@erwango Ideally the update hal script should take care of that step, I agree. However, I'd do that in a second stage. I can generate a README.rst file with the version/sha taken from each series README file. Does that sound good?

@erwango
Copy link
Member

erwango commented Oct 13, 2020

@erwango Ideally the update hal script should take care of that step, I agree. However, I'd do that in a second stage. I can generate a README.rst file with the version/sha taken from each series README file. Does that sound good?

For now it'll be fine, txs!

@gmarull gmarull requested a review from erwango October 13, 2020 09:08
@gmarull
Copy link
Member Author

gmarull commented Oct 13, 2020

@erwango generated README only includes version, as SHA1 is not present for all series.

@gmarull
Copy link
Member Author

gmarull commented Oct 21, 2020

@erwango ping

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Please update README section "How to introduce a new version of stm32cube:" to mention that script should be run systematically after a new STM32Cube package version is introduced.

In a second step, this should be made part of the update script directly.

@erwango
Copy link
Member

erwango commented Oct 22, 2020

Approved on my side, but would like potential out of tree users to review and approve before merge

Copy link
Member

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

LGTM. Only found a small typo which is not critical.

@gmarull gmarull requested review from ABOSTM and galak October 26, 2020 21:02
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

Please generate a new batch of headers based on the latest cube versions and we're good to go.

The script is able to generate common LL headers which can be used in
both Zephyr and applications to make it easier to create family
independent code.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Initial batch of auto-generated generic LL headers.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add a test to check that generated output is correct.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Reference the newly introduced generic LL headers.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
@gmarull gmarull requested a review from erwango November 2, 2020 20:14
@gmarull
Copy link
Member Author

gmarull commented Nov 5, 2020

@galak can we merge this one?

@galak galak merged commit a07d3af into zephyrproject-rtos:master Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants