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

3D printer board Big Tree Tech EBB42, missing Generic STM32G0 and STM32G4 support #1784

Merged
merged 3 commits into from
Aug 31, 2022
Merged

Conversation

alextrical
Copy link
Contributor

@alextrical alextrical commented Aug 6, 2022

Added new Generic support for STM32G0B1/C1 support, and 3D printer board Big Tree Tech EBB42 CANBUS V1.1 motor / hot end control board

Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

1 small comment, otherwise LGTM

Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks

Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

@alextrical,
Thanks, that is a huge work.
Few comments:

variants/STM32G0xx/G030J6M/generic_clock.c Outdated Show resolved Hide resolved
@alextrical
Copy link
Contributor Author

  • Could you run Astyle to harmonize formatting

I'm going to try that now, Once I download the ".astyleignore", ".astylerc" and "astyle.py" files, Do i run the py file at the root of the project, and do i need to manually specify in the command to use the definition files?

@ABOSTM
Copy link
Contributor

ABOSTM commented Aug 10, 2022

In fact scripts to ease Astyle are already available when cloning github:
https://github.com/stm32duino/Arduino_Core_STM32/tree/main/CI/astyle

But you need to download Astyle itself.

Then all we request is to run Astyle on the sources/headers files you modified.
So you can run for example :
> python3 CI/astyle/astyle.py -p /c/Arduino/AStyle/bin -r ./variants/STM32G0xx/
(suppose Astyle is installed in /c/Arduino/AStyle/bin)

@alextrical
Copy link
Contributor Author

I've run Astyle with the following command
python astyle.py -i .astyleignore -d .astylerc -r /var/home/alextrical/GitHub/Arduino_Core_STM32/variants/STM32G0xx
It looks to have made the required changes to the 'generic_clock.c' files in each file that has been created for this pull :)

Hopefully the requested changes are all now showing up.
i.e. Astyle formatting of 'generic_clock.c' / files
Values removed from the following for all boards added : RCC_ClkInitStruct = {}; & RCC_OscInitStruct = {};
Readme updated for all MCU's

I will remember these for the next batch ;) and leave it a bit longer between making the changes, and placing a change request to hopefully let it capture the full set of changes, and hopefully i will only create one pull request next time ;)

Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

Some remaining '0' to be removed, including in variant_EBB42_v1_1.cpp

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

LGTM
(if you wants to add MCU on other STM32 series, let's do it in separate commit 😃 )
Thanks for this huge and valuable work

@alextrical
Copy link
Contributor Author

alextrical commented Aug 10, 2022

How do I go about creating a separate commit, I was hoping the last Pull request for the G4 would have been a separate entity from the G0 submission. Is the Close with comment enough?

I'm not sure if the pull request fully checked in, it may need extra approval on your side.
image
https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

@alextrical
Copy link
Contributor Author

once this pull request is closed I will start adding a few other definitions, that are queued up and ready to go. Currently sitting in another repo for safe storage ready to be added.
https://github.com/alextrical/STM32-linker-scripts/tree/main/variants

@ABOSTM
Copy link
Contributor

ABOSTM commented Aug 11, 2022

Sorry, I mean separate Pull Request 😄
Yes, PR needs extra check, and at the end PR needs to be merged to be fully checked in.
Extra check includes CI automatic check which are currently failed (see above)

At first glance there is an error while compiling GENERIC_G0B0CETX, GENERIC_G0B0RETX and GENERIC_G0B0VETX
/github/home/.arduino15/packages/STMicroelectronics/hardware/stm32/2.3.0/cores/arduino/stm32/uart.h:120:21: error:

'USART3_4_IRQn' undeclared (first use in this function); did you mean 'USART4_IRQn'?
  120 | #define USART3_IRQn USART3_4_IRQn

I will have a look at this issue

@alextrical
Copy link
Contributor Author

alextrical commented Aug 11, 2022

ah, well that's a pain that its not compiling :/ Would you be OK with me removing the 3 failed MCU's from the PR?
I can see why this should be submitted as multiple smaller Pull requests 😅
unfortunately i made the changes directly into the Main branch of my fork, and now I don't know if its possible to create a branch without causing extra issues for future PR's

@ABOSTM
Copy link
Contributor

ABOSTM commented Aug 11, 2022

I made a fix for the compilation error #1790.
So I prefer you don't remove the failed MCU and thus have a complete G0 support.
Whatever, we will wait for @fpistm to review before merging.

Nevertheless, you can push other STM32 series in parallel (one per PR would be great).
Warning each PR should be independent one from each other, as long as they are not merged.
Exemple: if you push a new PR to add MCU for F1, it should not be on top of current G0 PR (as long as it is not merged) nor other potential PR for F0 series you have in the pipe ...

Alternatively, you can wait for this one to be merged, before pushing next PR. This will take more time to reach the end of STM32 series, but it is not an issue.
Please note that next week, support will be drastically reduced due to summer holidays.

@alextrical
Copy link
Contributor Author

alextrical commented Aug 11, 2022

I prefer you don't remove the failed MCU and thus have a complete G0 support.
That's fair enough, I will leave the G0B0xx series in place.

Nevertheless, you can push other STM32 series in parallel (one per PR would be great).
As for pushing independent PR's, I'm guessing that needs to be done with branches within my copy of the project, with 1 branch per MCU family.

if you push a new PR to add MCU for F1, it should not be on top of current G0 PR
agreed, I hadn't realised what I was doing wrong, I'm hoping to do the future PR's as independent chunks

Unfortunately I did all previous modifications directly in 'Main' so think I have somewhat contaminated the base of my fork

@ABOSTM
Copy link
Contributor

ABOSTM commented Aug 11, 2022

As for pushing independent PR's, I'm guessing that needs to be done with branches within my copy of the project, with 1 branch per MCU family.

Right

Unfortunately I did all previous modifications directly in 'Main' so think I have somewhat contaminated the base of my fork

Thus let's wait for merge of 1st PR before pushing next one.

ABOSTM
ABOSTM previously requested changes Aug 11, 2022
Copy link
Contributor

@ABOSTM ABOSTM left a comment

Choose a reason for hiding this comment

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

2.4.0

Copy link
Contributor Author

@alextrical alextrical left a comment

Choose a reason for hiding this comment

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

Version Number now updated to show 2.4.0 in readme.md

README.md Show resolved Hide resolved
@fpistm
Copy link
Member

fpistm commented Aug 23, 2022

Hi @alextrical

Could you rebase your PR on top of the main in order to get the fix made by @ABOSTM. Then CI should be OK
Thanks.

Pay attention as you made a PR using the main branch.

@alextrical
Copy link
Contributor Author

Hopefully that is now in sync with the main repo. Please give the CI a try

@fpistm fpistm added this to In progress in STM32 core based on ST HAL via automation Aug 31, 2022
@fpistm fpistm added this to the 2.4.0 milestone Aug 31, 2022
@fpistm fpistm changed the title Added STM32G0B1/C1 support, and 3D printer board Big Tree Tech EBB42 3D printer board Big Tree Tech EBB42, Generic STM32G0B1/C1 and STM32G4 support Aug 31, 2022
@fpistm
Copy link
Member

fpistm commented Aug 31, 2022

Hi @alextrical
I've clean up the branch as I could not simply squash it. Now only 3 commits.

So, please do not push on your main branch while this PR is not merged.

fpistm and others added 3 commits August 31, 2022 16:25
@fpistm fpistm changed the title 3D printer board Big Tree Tech EBB42, Generic STM32G0B1/C1 and STM32G4 support 3D printer board Big Tree Tech EBB42, missing Generic STM32G0 and STM32G4 support Aug 31, 2022
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

STM32 core based on ST HAL automation moved this from In progress to Reviewer approved Aug 31, 2022
@fpistm fpistm merged commit 9875d7d into stm32duino:main Aug 31, 2022
STM32 core based on ST HAL automation moved this from Reviewer approved to Done Aug 31, 2022
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

4 participants