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

Needed_changes_to_compile_with_gcc_13.2.1 #2224

Merged

Conversation

Brumi-2021
Copy link
Contributor

@Brumi-2021 Brumi-2021 commented Aug 14, 2024

Hi , to not forget the current investigation, (and thanks specially to @bernd-herzog , @u-foka , and @NotherNgineer ...)

Please find attached the changes that I need to apply , for been able to compile with the new version gcc 13.2.1 arm-none-eabi.
Till now I was using the version gcc 10.3.1, but recently pentoo linux updated it to the new gcc 13.2.1

Notes :
1-) Clearly , using the compiler gcc 13.2.1 (same as with the previous gcc 10.3.1) , those upgraded gcc versions produced with the same source code , a slighly bigger binary size , than the binary produced with the current official upstream 9.2.1.
(therefore in my compile test, following @NotherNgineer advices , I usually delete temporary some application to reduce the binary size , in ui_navigation.cpp )
Ex .
// {"sstvtx", "SSTV", TX, ui::Color::green(), &bitmap_icon_sstv, new ViewFactory()},
//{"touchtune", "TouchTune", TX, ui::Color::green(), &bitmap_icon_touchtunes, new ViewFactory()},

2-) I could compile all from vscode , using embeded CMake tool , (same as I was doing with 10.3.1 , thanks to @bernd-herzog help )
Now no problem, even it gives me that msg :
"[build] WARNING: Compiler version mismatch, please use the official compiler version 9.2.1 when sharing builds! Current compiler version: 13.2.1"

3-) Once reflashed the new compiled binary (created with gcc. 13.2.1) ,
I could confirm that using our Portapack , we can generate DEBUG_DUMP_XXX.TXT ,
and confirm with it , that the generated bin , comes from gcc a real 13.2.1 (just a double check of the used compiler version)
image

4-) Recently, I was getting some GURU mediation errors , just at power up with my binary compiled with gcc 13.2.1,
but all those problems have been dissappeared after re-sync my cloned github repo with latest today's next .
(I still do not know why , but now it works well , no more GURU errors)
https://github.com/user-attachments/assets/7bad6661-5196-4710-bc0c-6da2d782aa24

image

5-) Regarding the event_m0.cpp file changes , those two changes are not related to this gcc compatibility issue,
but trying to solve the above (4) , I added two lines , to delete two more warnings. And now all seems ok ,
But I will need your approval about them .
I could delete from 28 compile warnings (actual) --> 26 compile warnings (after this PR) .
Now all remaining 26 warnings , seems to be related to USB
image

6-) The applied changes in the file : /opt/portapack-mayhem/firmware/tools/make_spi_image.py
image

Are not strictly necessary .But due to the point (1) , initially I was getting so many uncompleted compilations with exit code(1) , and it was so annoying and I could not test the full compile and link process.
Then , in my set up investigation , I decided to comment those two below lines ,
It is true that now when excedding 1MB binary flash size , we will not get the usual error message :
Example,
image

But anyway , we still could always check that other output line .
And if the room margin is negative (-) , means that we are exceding the 1MB flash size. (we do not have positive (+) room margin)
image

Post-note about (6) ,
based on above revision comments, I have re-activated that Binary limit size protection check with a second commit inside that PR, (as it was originally)


There is no rush to be merged that PR .
But soon or later gcc version 9.2.1 might be obsolete ...
But before merging it , we need to be completely sure that this PR is fully compatible with current upstream 9.2.1 compilation , without breaking anything . (Hopefully it should be compatible and transparent) .

Waiting for your testing support and comments .

@u-foka , in the past , I know that you were also using gcc version 12,x.x...or even 13.x.x ,
so maybe you can also have a chance to test those changes ...

Thanks in advance for your test , comment reviews , and feedbacks
Cheers.

Copy link
Contributor

@NotherNgineer NotherNgineer left a comment

Choose a reason for hiding this comment

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

I fully agree with the goal to enable alternative gcc versions for Mayhem testing.

@gullradriel
Copy link
Member

gullradriel commented Aug 15, 2024

Nice ! When will you undraft it ?

Copy link
Contributor

@u-foka u-foka left a comment

Choose a reason for hiding this comment

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

Looks promising, great work thanks! :)

firmware/standalone/pacman/CMakeLists.txt Show resolved Hide resolved
@Brumi-2021 Brumi-2021 marked this pull request as ready for review August 15, 2024 15:08
@Brumi-2021
Copy link
Contributor Author

I resolved both previous conversations .
I added one new commit , changing back the Limit flash size protection of 1MB .

So far , I am not deleting the double compile options specs. (I know that it was already added in yesterday @NotherNgineer's PR : Fix "standalone pacman" compile errors with gcc >9.2.1 #2223) .
This PR has exactly same change , hopefully it will not create any conflict . If not , we will amend it .

Copy link
Contributor

@NotherNgineer NotherNgineer left a comment

Choose a reason for hiding this comment

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

lgtm

@u-foka
Copy link
Contributor

u-foka commented Aug 15, 2024

Looks good to me thanks!

@gullradriel gullradriel merged commit 6dc7e3d into portapack-mayhem:next Aug 15, 2024
3 checks passed
@gullradriel
Copy link
Member

Merged,
Many thanks to all ;-)

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.

4 participants