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

test(PIO):improving SET PINS test to be unaffected by other pins #65

Merged
merged 3 commits into from
Jun 5, 2021

Conversation

Turro75
Copy link
Contributor

@Turro75 Turro75 commented Jun 4, 2021

the set pins requires the result and-ed to avoid the other pins status

@Turro75
Copy link
Contributor Author

Turro75 commented Jun 4, 2021

as alternative the DBG_PADOUT can be reset by writing 0 to PINS with a MOV instruction, I don't know if DBG_PADOUT is affected by GPIO status, anyway for testing purpose seems not important.

 async function resetStateMachines() {

    await cpu.writeUint32(CTRL, 0xf0);
    await cpu.writeUint32(SM0_INSTR, pioJMP(PIO_COND_ALWAYS, 0)); // Jump machine 0 to address 0

    // Clear FIFOs
    await cpu.writeUint32(SM0_SHIFTCTRL, FJOIN_RX);  
    await cpu.writeUint32(SM0_SHIFTCTRL, 0);

    //reset Pins/DBG_PADOUT
    await cpu.writeUint32(SM0_INSTR, pioSET(PIO_DEST_X, 0));
    await cpu.writeUint32(SM0_INSTR, pioMOV(PIO_DEST_PINS, PIO_OP_NONE, PIO_SRC_X)); 
  }

expect(await cpu.readUint32(DBG_PADOUT)).toBe(13);
await cpu.writeUint32(SM0_PINCTRL, (5 << SET_COUNT_SHIFT) | (shiftAmount << SET_COUNT_BASE));
await cpu.writeUint32(SM0_INSTR, pioSET(PIO_DEST_PINS, pinValue));
expect((await cpu.readUint32(DBG_PADOUT)) & (pinValue << shiftAmount)).toBe(
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, we want to check all the pins (and not only the ones we've set to 1), so we need the mask to be 0x1f and not pinValue, e.g.:

    expect((await cpu.readUint32(DBG_PADOUT)) & (0x1f << shiftAmount)).toBe(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I pushed another commit

@urish
Copy link
Contributor

urish commented Jun 4, 2021

Thank you Valerio!

I don't know if DBG_PADOUT is affected by GPIO status, anyway for testing purpose seems not important.

The datasheet says: "Read to sample the pad output values PIO is currently driving to the GPIOs.", so as I understand it it's only affected by PIO and not by any external peripherals.

In any case, both methods are fine, I don't have any strong preference for one over the other.

…lved pins

reset: set the reset value of registers
@Turro75
Copy link
Contributor Author

Turro75 commented Jun 4, 2021

In any case, both methods are fine, I don't have any strong preference for one over the other.

me neither, both work.
a little bit better the reworked test as it's more generic and allows the testing of any combination of SET PINS while the other way uses instructions to test instructions, but who cares it's a test and now it passes.

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.

Awesome! Just one small request :)

await cpu.writeUint32(SM0_SHIFTCTRL, 0);
//values at reset
await cpu.writeUint32(SM0_SHIFTCTRL, (1 << 18) | (1 << 19));
await cpu.writeUint32(SM0_PINCTRL, 5 << SET_COUNT_SHIFT);
}

// TODO figure out why this specific test fails on silicone?
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also remove this TODO comment (it doesn't fail anymore!)

@@ -101,17 +102,27 @@ describe('PIO', () => {
await cpu.writeUint32(SM0_INSTR, pioJMP(PIO_COND_ALWAYS, 0)); // Jump machine 0 to address 0
// Clear FIFOs
await cpu.writeUint32(SM0_SHIFTCTRL, FJOIN_RX);
await cpu.writeUint32(SM0_SHIFTCTRL, 0);
//values at reset
await cpu.writeUint32(SM0_SHIFTCTRL, (1 << 18) | (1 << 19));
Copy link
Contributor

Choose a reason for hiding this comment

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

What are do bit 18 / 19 mean?

Let's define a constant for them (like we have FJOIN_RX), so the test code is easier to understand

resetMachineState: set proper name to constant values used at reset
@urish urish merged commit 0b0ea10 into wokwi:master Jun 5, 2021
@urish
Copy link
Contributor

urish commented Jun 5, 2021

Thanks!

@Turro75 Turro75 deleted the improve-SET-PINS-test branch June 5, 2021 17:58
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