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

[READY] Reduce hex size #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mcgalliard
Copy link
Contributor

@mcgalliard mcgalliard commented Sep 2, 2024

6 months ago gap fill was added to post build hex copying. This caused some hex inflation - up to 1GB....
This needed to be removed

@zyonse
Copy link
Contributor

zyonse commented Sep 2, 2024

Why was gap fill originally enabled?

@mcgalliard
Copy link
Contributor Author

Why was gap fill originally enabled?
See https://github.com/PurdueElectricRacing/firmware/commit/1248be27d0e4654c988086544c2231785b8e73e2
"Main module output had multiple segments (only a few bytes away). The bootloader doesn't support jumps in address, so we will just send some zeros to fill the gap"

We will need to add bootloader support for jumps in address

@mcgalliard mcgalliard changed the base branch from master to vehicle/2025 September 2, 2024 15:56
zyonse
zyonse previously approved these changes Sep 2, 2024
Copy link
Contributor

@AdityaAsGithub AdityaAsGithub left a comment

Choose a reason for hiding this comment

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

Underlying issue necessitating gap fill should be fixed before this gets merged into master.

@mcgalliard mcgalliard changed the base branch from vehicle/2025 to master September 2, 2024 18:04
@ColexDev
Copy link
Member

ColexDev commented Sep 3, 2024

The link you sent in response to Gavin leads to a 404 for me. I would also like to see the original reasoning behind why we added the gap-fill flag as well.

The bootloader doesn't support jumps in address

What? Why is this? The bootloader is not able to jump and has to run sequentially?

@zyonse
Copy link
Contributor

zyonse commented Sep 3, 2024

@ColexDev this is the commit he linked: 1248be2

AdityaAsGithub
AdityaAsGithub previously approved these changes Sep 7, 2024
@AdityaAsGithub AdityaAsGithub dismissed their stale review September 7, 2024 06:32

im testing sum out

ColexDev
ColexDev previously approved these changes Sep 8, 2024
irvingywang
irvingywang previously approved these changes Sep 8, 2024
@AdityaAsGithub AdityaAsGithub changed the base branch from master to feature/anand89/permissions_testing September 11, 2024 07:03
@AdityaAsGithub AdityaAsGithub changed the base branch from feature/anand89/permissions_testing to master September 11, 2024 07:04
@AdityaAsGithub AdityaAsGithub dismissed stale reviews from irvingywang, ColexDev, and zyonse September 11, 2024 07:04

The base branch was changed.

@AdityaAsGithub AdityaAsGithub changed the base branch from master to feature/anand89/permissions_testing September 11, 2024 07:12
@AdityaAsGithub AdityaAsGithub changed the base branch from feature/anand89/permissions_testing to master September 11, 2024 07:13
Copy link
Contributor

@AdityaAsGithub AdityaAsGithub left a comment

Choose a reason for hiding this comment

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

This MR should not merge, for the same reason I mentioned earlier

@AdityaAsGithub AdityaAsGithub dismissed their stale review September 12, 2024 04:39

Testing something out I still don't this

AdityaAsGithub
AdityaAsGithub previously approved these changes Sep 12, 2024
@AdityaAsGithub AdityaAsGithub dismissed their stale review September 12, 2024 04:40

I was testing the CODEOWNERS file

LukeOxley
LukeOxley previously approved these changes Sep 12, 2024
Copy link
Member

@LukeOxley LukeOxley left a comment

Choose a reason for hiding this comment

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

Test :•)

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.

6 participants