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

Patch sm #1519

Merged
merged 1 commit into from
Oct 29, 2021
Merged

Patch sm #1519

merged 1 commit into from
Oct 29, 2021

Conversation

beserge
Copy link
Contributor

@beserge beserge commented Oct 19, 2021

Summary

This PR fixes/implements the following bugs/features

  • Add Daisy Patch SM variant

This PR adds the Daisy Patch SM as a variant.

The Patch SM is an upcoming hardware platform from Electrosmith following in the footsteps of the Daisy Seed.

The one thing we might want to change is the ordering of the Analog pins (A0, A1, etc).
We tried simply reordering the analogInputPin array, which worked for some pins, but eventually just broke.

@beserge beserge marked this pull request as draft October 19, 2021 22:12
@beserge beserge marked this pull request as ready for review October 21, 2021 15:43
@fpistm fpistm self-requested a review October 22, 2021 17:11
@fpistm fpistm added this to In progress in STM32 core based on ST HAL via automation Oct 22, 2021
@fpistm
Copy link
Member

fpistm commented Oct 22, 2021

Hi @beserge
I will review it next week.
I guess it should be fine to wait the final board version? or is it ready and the pin-out will not change?

@beserge
Copy link
Contributor Author

beserge commented Oct 22, 2021

@fpistm The hardware is finalized, so the pinout won't change. Do you have any advice for setting up those Analog Pins I mentioned?

@fpistm
Copy link
Member

fpistm commented Oct 22, 2021

I wiil check deeply on monday.

the PIN_Ax defined in the .h should match in the analogInputPin[] of the .c:
If you revert PIN_A1 and PIN_A2, then the number in the array have to be reverted also.
The number is the index in the digitalPin array in the .c.

@beserge
Copy link
Contributor Author

beserge commented Oct 22, 2021

@fpistm I was able to get them working, thanks for the help!

Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

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

PR LGTM
Anyway it misses:

  • a reference in the README.md
  • Clean up useless pin definitions
  • Squash all commit in one

Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

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

STM32 core based on ST HAL automation moved this from In progress to Needs review Oct 26, 2021
@beserge beserge closed this Oct 26, 2021
STM32 core based on ST HAL automation moved this from Needs review to Done Oct 26, 2021
@beserge beserge mentioned this pull request Oct 27, 2021
@beserge beserge reopened this Oct 27, 2021
STM32 core based on ST HAL automation moved this from Done to In progress Oct 27, 2021
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
STM32 core based on ST HAL automation moved this from In progress to Needs review Oct 27, 2021
Create variant .h .cpp and pinperiphs.c files
Use daisy seed linker script for patch sm
Add variant to boards.txt
Add Patch SM with link to README.md
STM32 core based on ST HAL automation moved this from Needs review to Reviewer approved Oct 27, 2021
Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

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

LGTM
Is it OK to merge?

@stephenhensley
Copy link

Everything checks out on our end. OK to merge.

Thanks for all the thorough reviews.

@fpistm fpistm merged commit 342cda8 into stm32duino:main Oct 29, 2021
STM32 core based on ST HAL automation moved this from Reviewer approved to Done Oct 29, 2021
@fpistm
Copy link
Member

fpistm commented Oct 29, 2021

Thanks for all the thorough reviews.

Welcome.
Thank you for following them 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants