Skip to content
This repository has been archived by the owner on Jun 13, 2023. It is now read-only.

make GPIO handling more generic & explicit #114

Open
Lotterleben opened this issue Mar 23, 2021 · 1 comment
Open

make GPIO handling more generic & explicit #114

Lotterleben opened this issue Mar 23, 2021 · 1 comment

Comments

@Lotterleben
Copy link
Contributor

Opening this PR mainly for documentation and to discuss objections/changes early on (ish).

IMO, the current API for writing GPIO tests is a bit opaque: which pin is set by calling test_stand.assistant.set_pin_low()? How would I even refer to these pins if I were to add more? Could we reconfigure this pin from our test code, during runtime since we have dynamic pins now?

I'd like to propose a new API (documented at the end of this issue).
I already have running code that implements this both for the host API as well as the T-A, but it's a lot of different changes:

  • host API rearrangement
  • introduction of dynamic pins on host and T-A
  • introduction of arduino-style consecutive pin numbering
  • introduction of non-interrupt driven dynamic pins on T-A as well while we're at it
  • decoupling of Test-Assistant and Test-Target (so you can test any other random device as well)
    and probably a pain to review in one go.

So, I'd start by splitting off a PR that only introduces the Test API below– without adding new functionality to the T-A. This may include adding some temporary shims like rejecting all pin numbers in create_gpio_output_pin() that aren't 29 (i.e. the red LED) that can be taken out when implementing the T-A side changes.

Does that work for you? What are your thoughts?


We've designed the interface so that

  • it is visible which pin is being changed/read by looking at the function call
  • visual noise is kept down
  • you're stopped at compile time if you try to do something you shouldn't (read an output pin for example)
  • pins are named by simple consecutive numbering, i.e. the gray numbers 1-40 in this diagram:

LPC845 Pinout Diagram

To illustrate this, let's look at the test in test-suite/tests/gpio.rs checking whether voltage changes at the test-assistant's PIO1_2 pin are registered by the target.
Before, it looked like this:

#[test]
fn it_should_read_input_level() -> Result {
    let mut test_stand = TestStand::new()?;

    test_stand.assistant.set_pin_low()?;
    //                   ^^^^^^^^^^^^^
    //                   unclear: which pin?
    assert!(test_stand.target.pin_is_low()?);

    // [...]

    Ok(())
}

Now, it looks like this:

const RED_LED_PIN: PinNumber = 29;
//                             ^^
//                             simpler pin numbering

#[test]
fn target_should_read_input_level() -> Result {
    // SETUP
    let mut test_stand = TestStand::new()?;
    let mut out_pin = test_stand
    //      ^^^^^^
    //      is of type `OutputPin`
        .assistant
        .create_gpio_output_pin(RED_LED_PIN, Level::High)?;
    //                          ^^^^^^^^^^^  ^^^^^^^^^^^
    //                    pin used for test  set initial pin
    //                                       voltage level so
    //                                       that pin is off


    // RUN TEST
    out_pin.set_low()?;
    //      ^^^^^^^
    //      can only be called on `OutputPin`s
    //      sets pin voltage level to 🚨

    assert!(test_stand.target.pin_is_low()?); // note: since `test-target` hasn't
                                              // been modified, this hasn't changed 

    // [...]
    // for demonstration purposes, let's add some more code:

    let in_pin = out_pin.into_input_pin()?;
    //  ^^^^^
    //  is now of type `InputPin`
    
    // this mistake will be prevented at compile time!
    in_pin.set_low()?;
    //     ^^^^^^^
    //     trying to set the voltage of an input pin 🛑 

    Ok(())
}
@hannobraun
Copy link
Owner

Thanks for opening this issue! Everything you suggest here definitely goes into the right direction. And yes, the current API certainly isn't ideal. There was a time, early on, when there a single pin used in the test setup. So the API talking about "the pin" made sense back then. Somehow, this survived until now.

I agree that splitting your changes into manageable slices is the best approach. Temporary measures that aid in the transition are perfectly acceptable.

One additional note: I think the pin numbering scheme you suggest makes perfect sense for the generic test assistant API. I always imagined that a test suite would wrap that generic API, to create a suite-specific API that is optimized for test readability and hides unused complexity. Such a wrapper might use specific terms (like the red LED pin from your example) in method names.

But I'm just mentioning that to clarify my initial intent. Whether to create such a suite-specific wrapper is a stylistic choice, and up to the author of the test suite.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants