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

Fix Freeboy noise channel playback for srw = 1. #4538

Merged
merged 1 commit into from Aug 16, 2018

Conversation

Projects
None yet
3 participants
@BonsaiDen
Contributor

BonsaiDen commented Aug 13, 2018

In order for GB_apu to correctly generate samples when the LSFR width is
set to 7, the trigger write to 0xff23 must happen after all other
writes.

@BonsaiDen

This comment has been minimized.

Show comment
Hide comment
@BonsaiDen

BonsaiDen Aug 13, 2018

Contributor

Oops, seems like some debug include got left in there. Will be removing that later today.

Contributor

BonsaiDen commented Aug 13, 2018

Oops, seems like some debug include got left in there. Will be removing that later today.

Fix Freeboy noise channel playback for srw = 1.
In order for `GB_apu` to correctly generate samples when the LSFR width is
set to `7`, the trigger write to `0xff23` must happen after all other
writes.
@SecondFlight

This comment has been minimized.

Show comment
Hide comment
@SecondFlight

SecondFlight Aug 13, 2018

Member

@PhysSong This looks like a pretty simple fix. Do you want to merge this into 1.2 since it's technically not a feature or defer it to 1.3?

@BonsaiDen Could you give a bit more information at a higher level as to what issue this PR solves?

Member

SecondFlight commented Aug 13, 2018

@PhysSong This looks like a pretty simple fix. Do you want to merge this into 1.2 since it's technically not a feature or defer it to 1.3?

@BonsaiDen Could you give a bit more information at a higher level as to what issue this PR solves?

@BonsaiDen

This comment has been minimized.

Show comment
Hide comment
@BonsaiDen

BonsaiDen Aug 13, 2018

Contributor

@SecondFlight Sure thing.

If you set up channel 4 with the SRW value of 15 you get the expected noisy output:

noise_srw15

However, if you switch the SRW value to 7 you get the following output (basically just two clicks):

noise_srw7

What would be expected instead is a similar waveform but with a "faster" sounding noise.

The issues arises from the way the APU registers on the GameBoy must be accessed. Writing the enable bit (1000_0000) to FF23 causes the noise oscillator to start outputting samples and writing the other registers (including the one for the SRW value) during the time the oscillator is active has undefined behaviour.

This is what causes the "silent" output above. Moving the output enable write so it occurs after the noise channel registers have all been set up (as in the PR) fixes this issue :)

Contributor

BonsaiDen commented Aug 13, 2018

@SecondFlight Sure thing.

If you set up channel 4 with the SRW value of 15 you get the expected noisy output:

noise_srw15

However, if you switch the SRW value to 7 you get the following output (basically just two clicks):

noise_srw7

What would be expected instead is a similar waveform but with a "faster" sounding noise.

The issues arises from the way the APU registers on the GameBoy must be accessed. Writing the enable bit (1000_0000) to FF23 causes the noise oscillator to start outputting samples and writing the other registers (including the one for the SRW value) during the time the oscillator is active has undefined behaviour.

This is what causes the "silent" output above. Moving the output enable write so it occurs after the noise channel registers have all been set up (as in the PR) fixes this issue :)

@SecondFlight

This comment has been minimized.

Show comment
Hide comment
@SecondFlight

SecondFlight Aug 13, 2018

Member

@BonsaiDen Thank you for the detailed explanation!

Member

SecondFlight commented Aug 13, 2018

@BonsaiDen Thank you for the detailed explanation!

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Aug 14, 2018

Member

Read the CPU's manual and looks good. The only concerns are possible minor compatibility issues.
I think we can merge this into stable-1.2. Any objections?

Member

PhysSong commented Aug 14, 2018

Read the CPU's manual and looks good. The only concerns are possible minor compatibility issues.
I think we can merge this into stable-1.2. Any objections?

@PhysSong PhysSong merged commit 0cddc46 into LMMS:stable-1.2 Aug 16, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment