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

fix(rp2040): Fix CMP reg T1 #31

Merged
merged 9 commits into from
May 16, 2021
Merged

fix(rp2040): Fix CMP reg T1 #31

merged 9 commits into from
May 16, 2021

Conversation

Turro75
Copy link
Contributor

@Turro75 Turro75 commented May 16, 2021

found a specific test where CMPregT1 wrongly set the carry

@Turro75
Copy link
Contributor Author

Turro75 commented May 16, 2021

Please be patient, one Day I'll learn how to PR only the relevant commits...
0cdd227 is the right one

Copy link
Contributor

@urish urish left a comment

Choose a reason for hiding this comment

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

Thanks for finding all these small flag bugs in the emulator :)

I've added some comments!

src/instructions.spec.ts Show resolved Hide resolved
await cpu.writeUint16(0x20000000, opcodeCMPregT1(r2, r0));
await cpu.setRegisters({ r0: 0xb71b0000 });
await cpu.setRegisters({ r2: 0x00b71b00 });
await cpu.setRegisters({ xPSR: 0x61000000 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of setting xPSR directly, let's set the specific flags (like we did in the "should execute add sp, r8 instruction and not update the flags" test case)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we can have a single call to setRegisters setting all the registers at once (instead of one call per register)

@@ -469,6 +469,19 @@ describe('Cortex-M0+ Instruction Set', () => {
expect(registers.V).toEqual(false);
});

it('should execute an `cmp r2, r0` instruction', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make the test name more specific, like: "should execute an cmp r2, r0 instruction and not set any flags when r0=0xb71b0000 and r2=0x00b71b00"

@urish
Copy link
Contributor

urish commented May 16, 2021

Please be patient, one Day I'll learn how to PR only the relevant commits...

if you need any help with that let me know

@@ -469,6 +469,18 @@ describe('Cortex-M0+ Instruction Set', () => {
expect(registers.V).toEqual(false);
});

it('should execute an cmp r2, r0 instruction and not set any flags when r0=0xb71b0000 and r2=0x00b71b00', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, one more small thing: please add backticks (`) around the instruction name, like in the other tests

@urish urish merged commit 813cdca into wokwi:master May 16, 2021
@Turro75 Turro75 deleted the patch-1 branch May 17, 2021 09:39
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.

2 participants