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

Scramble banks from the end of the ROM #1273

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Dec 12, 2023

This is more likely to test edge cases, such as having content in banks with their highest bit set.

Fixes #1036.

#1149 proposes allocating sections from the end of their individual banks. This is a similar approach, allocating section banks from the end of their overall ROM space.

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgblink This affects RGBLINK labels Dec 12, 2023
@Rangi42 Rangi42 added this to the v0.7.0 milestone Dec 12, 2023
@Rangi42 Rangi42 requested a review from ISSOtm December 12, 2023 20:05
ISSOtm
ISSOtm previously requested changes Dec 12, 2023
test/link/test.sh Outdated Show resolved Hide resolved
@Rangi42 Rangi42 requested a review from ISSOtm December 12, 2023 20:24
@Rangi42 Rangi42 marked this pull request as draft December 12, 2023 20:33
@Rangi42 Rangi42 removed this from the v0.7.0 milestone Dec 14, 2023
ISSOtm
ISSOtm previously approved these changes Dec 14, 2023
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@Rangi42 Rangi42 dismissed ISSOtm’s stale review December 16, 2023 03:27

Not actually good yet

@Rangi42 Rangi42 force-pushed the scramble-backwards branch 2 times, most recently from 2f3a97f to c2aafe7 Compare December 16, 2023 14:42
@Rangi42 Rangi42 requested a review from ISSOtm December 16, 2023 14:44
@Rangi42 Rangi42 marked this pull request as ready for review December 16, 2023 14:44
@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 16, 2023

Okay, now trying the next bank is updated too, not just assigning the first bank. Say you pass -S romx=3: then it will try the ROMX banks in order 3, 2, 1, 4, 5, 6, 7, .... Or -S sram=4: they'll go 4, 3, 2, 1, 0, 5, 6, 7, .... (And without scrambling they'll just ascend starting from firstBank.)

ISSOtm
ISSOtm previously requested changes Dec 16, 2023
src/link/assign.cpp Show resolved Hide resolved
test/link/test.sh Outdated Show resolved Hide resolved
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Subject to testing by @eievui5 before release, preferably?

@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 17, 2023

The macOS tests give this error:

./test.sh: line 14: rm -f ${otemp@Q} ${gbtemp@Q} ${gbtemp2@Q} ${outtemp@Q}: bad substitution

It's complaining about this line:

trap "rm -f ${otemp@Q} ${gbtemp@Q} ${gbtemp2@Q} ${outtemp@Q}" EXIT

I haven't changed the definitions of those temp files, so not sure what the actual problem is.

My hypothesis would be that this line is responsible, since the rest of the new test is straightforwardly like the others:

rom_size=$(wc -c <"$gbtemp")

Except that tryCmpRom does the same thing without issue.

Edit: Turns out macOS needed the printf after all; even doing wc -c <filename instead of wc -c filename, it doesn't just print the size in digits.

@Rangi42 Rangi42 added this to the v0.7.0 milestone Dec 17, 2023
This is more likely to test edge cases, such as having
content in banks with their highest bit set.
@Rangi42
Copy link
Contributor Author

Rangi42 commented Dec 18, 2023

Subject to testing by @eievui5 before release, preferably?

Tested and approved!

@Rangi42 Rangi42 merged commit 3901817 into gbdev:master Dec 18, 2023
23 checks passed
@Rangi42 Rangi42 deleted the scramble-backwards branch December 18, 2023 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs rgblink This affects RGBLINK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update scramble to specially handle >8-bit banks
2 participants