Skip to content

Add stm to docs matrix #2598

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

Merged
merged 7 commits into from
Apr 24, 2020
Merged

Add stm to docs matrix #2598

merged 7 commits into from
Apr 24, 2020

Conversation

hierophect
Copy link
Collaborator

@hierophect hierophect commented Feb 6, 2020

As per title. Removes a number of build flags as redundant and reworks the stm port mpconfigport.mk to match. Adjusts shared_bindings_matrix.py to recognize STM32 family categories. Adds a minor addition to porting.rst to explain how build flags impact porting.

Builds locally without issue, and correctly lists modules for all STM32 boards in the support matrix list, including the thunderpack (which enables NVM) and the H7 and F7 (which have restricted modules).

Resolves #2587
Resolves #2458

@hierophect hierophect requested a review from sommersoft February 6, 2020 20:19
sommersoft
sommersoft previously approved these changes Feb 6, 2020
Copy link

@sommersoft sommersoft left a comment

Choose a reason for hiding this comment

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

I also built locally, and everything¹ seems to be turning out correctly. Thanks @hierophect!

¹ There appears to be some deeper brokenness going on. Things like frequencyio are showing up as available on the STM32F4s. They shouldn't be, since its not included in a MINIMAL_BUILD and I don't see it explicitly turned on anywhere. That is mostly outside of this though, so I'll find a better way to document the issue.

@sommersoft
Copy link

I meant to add: I'll hold off on merging. I'm not a huge fan of sending known-incorrect documentation out. Without a better grasp on what is causing the noted problem, I'm not sure how a long a fix would take.

Thoughts on holding this until I (or whomever) gets a handle on the secondary problem?

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 7, 2020

Are there similar issues on other ports, or just stm32f4?

@sommersoft
Copy link

@dhalbert, looking at samples for different ports, I'm not seeing similar issues.

I'm pretty confident that its only related to the use of CIRCUITPY_MINIMAL_BUILD. The minimal build logic was added after docs/shared_bindings_matrix.py was written, so it just doesn't handle it.

@dhalbert
Copy link
Collaborator

OK, I will add a 'changes requested' review to hold off on this until the .py script is fixed.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Let's hold off until this is fixed:
@sommersoft sys:

I'm pretty confident that its only related to the use of CIRCUITPY_MINIMAL_BUILD. The minimal build logic was added after docs/shared_bindings_matrix.py was written, so it just doesn't handle it.

@hierophect
Copy link
Collaborator Author

@dhalbert do you think it'd be worth it for me to address #2458 as a part of this? Just do it all in one go?

@dhalbert
Copy link
Collaborator

@hierophect I think so, if you can get it to work, since then the doc will be correct from the start.

@hierophect hierophect changed the title Add stm32f4 to docs matrix Add stm to docs matrix Apr 22, 2020
@hierophect
Copy link
Collaborator Author

@sommersoft I've adjusted the shared_bindings_matrix.py file - builds ok for me but let me know if you'd like to see any changes.

@jepler could you double check the change I made to MICROPY_COMP_FSTRING_LITERAL in circuitpy_mpconfig.h? I removed MINIMAL_BUILD as a flag - are there still conditions where you'd like this to be optional?

sommersoft
sommersoft previously approved these changes Apr 22, 2020
Copy link

@sommersoft sommersoft left a comment

Choose a reason for hiding this comment

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

I built locally, a couple times. Verified that the ifeq ($(MCU_SERIES),F4) patterns in the STM port's mpconfigport.mk work, should they start to drift. Its all 5x5 that I can see.

I kind of liked the ALWAYS/DEFAULT/MINIMAL approach, but I spend no time adding ports. I'll agree with any approach of those that do. 😄

There are a few cleanup things, but they're mostly my doing from the initial effort. I have no issues handling them myself, as I prepare to loop shared_bindings_matrix.py into providing circuitpython.org with this info.

Thanks @hierophect for getting the STMs into the docs!

@hierophect
Copy link
Collaborator Author

hierophect commented Apr 22, 2020

@sommersoft I liked the concept of ALWAYS/DEFAULT/MINIMAL at first, but I realized it was backwards in how it affected ports. Setting everything to off and then adding what you support meant that you had to memorize what was in each "level" and then constantly re-evaluate if you should be moving up to the next.

Instead, starting at 100% and having a list of everything you turn off is like having a built in TODO list, one that you continually whittle down at as the port progresses (which is what everyone but me was doing anyway). I posted an example in porting.rst so other ports can get an idea of what that's like. I ultimately think it's better this way, with no categories at all except the one that SAMD21G18s need to restrict flash size.

@hierophect
Copy link
Collaborator Author

@dhalbert could you give this another look?

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Good explanations in the new comments. I had a few corrections, and some queries.

@sommersoft
Copy link

@dhalbert the raw strings were part of the cleanup I mentioned in my review. Nothing wrong with fixing them here; I just figured I'd clean up my own messes. 😄

@hierophect
Copy link
Collaborator Author

@dhalbert ok fixes are in and merge completed. I went ahead and deleted SMALL_BUILD completely across the port, atmel simply sets FULL_BUILD=0 now.

@hierophect hierophect requested a review from dhalbert April 23, 2020 17:42
@tannewt tannewt removed their request for review April 23, 2020 18:57
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

ok, getting really close!

@hierophect hierophect requested a review from dhalbert April 23, 2020 21:13
@hierophect
Copy link
Collaborator Author

@dhalbert should be all wrapped up for one last review!

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

OK, looks great, thanks for all the work on this!

@dhalbert dhalbert merged commit 5a22a5d into adafruit:master Apr 24, 2020
@hierophect hierophect deleted the stm32-docfix branch April 29, 2020 17:30
@hierophect hierophect added the stm label Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add The STM32 Port To Docs Module Support Matrix Create more useful BUILD flags
4 participants