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

Compile-time MPSSE command array construction macro #41

Merged
merged 5 commits into from
Jun 19, 2021

Conversation

CirrusNeptune
Copy link
Contributor

For situations where dynamic MPSSE command data layout is not necessary, a declarative macro may be used to safely construct a valid command array at compile-time.

This macro provides implementations of all commands except for set_clock. It parses a rust-like syntax with additional conveniences (i.e. const bindings have fixed-length array types specified automatically).

Additional details are provided in the doc comment for the macro.

@newAM
Copy link
Member

newAM commented May 29, 2021

Force pushed to rebase onto the latest origin/main before I review.

@CirrusNeptune
Copy link
Contributor Author

CirrusNeptune commented May 29, 2021

I just realized you can also leverage macro shadowing in a user crate to do command abstractions.

I could include this in the docs and the following test if you think it would be a valuable mention.

#[test]
fn user_abstracted_macro() {
    macro_rules! mpsse {
        // Practical abstraction for SPI devices.
        (@intern $passthru:tt cs_low(); $($tail:tt)*) => {
            mpsse!(@intern $passthru set_gpio_lower(0x0, 0xb); $($tail)*);
        };
        (@intern $passthru:tt cs_high(); $($tail:tt)*) => {
            mpsse!(@intern $passthru set_gpio_lower(0x8, 0xb); $($tail)*);
        };

        // Everything else handled by crate implementation.
        ($($tokens:tt)*) => {
            libftd2xx::mpsse!($($tokens)*)
        };
    }

    mpsse! {
        const (DATA, DATA_READ_LEN) = {
            wait_on_io_high();
            cs_low();
            wait_on_io_low();
            const DATA_RANGE = clock_data(ClockData::MsbPosIn, [11, 22, 33, 44]);
            cs_high();
            send_immediate();
        };
    }
    assert_eq!(DATA.len(), 16);
    assert_eq!(
        DATA,
        [
            MpsseCmd::WaitOnIOHigh as u8,
            MpsseCmd::SetDataBitsLowbyte as u8,
            0x0,
            0xb,
            MpsseCmd::WaitOnIOLow as u8,
            ClockData::MsbPosIn as u8,
            4 as u8,
            0 as u8,
            11 as u8,
            22 as u8,
            33 as u8,
            44 as u8,
            MpsseCmd::SetDataBitsLowbyte as u8,
            0x8,
            0xb,
            MpsseCmd::SendImmediate as u8,
        ]
    );
    assert_eq!(DATA_READ_LEN, 4);
    assert_eq!(DATA_RANGE, 0..4);
}

Copy link
Member

@newAM newAM left a comment

Choose a reason for hiding this comment

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

Took a while to read and understand all that macro code, picked up a few new things along the way, pretty elegant!

I added comments for a few minor changes, but otherwise looks good to me.

src/mpsse.rs Outdated Show resolved Hide resolved
src/mpsse.rs Outdated Show resolved Hide resolved
src/mpsse.rs Outdated Show resolved Hide resolved
src/mpsse.rs Show resolved Hide resolved
@newAM
Copy link
Member

newAM commented May 29, 2021

I just realized you can also leverage macro shadowing in a user crate to do command abstractions.

I could include this in the docs and the following test if you think it would be a valuable mention.

Just saw this after the review; that is really useful and I think it would help some people out!

@CirrusNeptune
Copy link
Contributor Author

CirrusNeptune commented May 29, 2021

Took a while to read and understand all that macro code, picked up a few new things along the way, pretty elegant!

I highly recommend looking through the examples in https://danielkeep.github.io/tlborm/book/ there are all kinds of useful patterns you should be aware of when designing a declarative macro.

I'll get on implementing those changes in a bit here.

Just saw this after the review; that is really useful and I think it would help some people out!

Should I go ahead and also add the following section to the macro doc?

/// # User Abstractions
///
/// With macro shadowing, it is possible to extend the macro with additional rules for abstract,
/// device-specific commands.
///
/// Comments within the implementation of this macro contain hints on how to implement these rules.
///
/// For example, a SPI device typically delineates transfers with the CS line. Fundamental
/// commands like `cs_high` and `cs_low` can be implmented this way, along with other
/// device-specific abstractions.
///
/// ```no_run
/// # use libftd2xx::mpsse;
/// macro_rules! mpsse {
///     // Practical abstraction of CS line for SPI devices.
///     (@intern $passthru:tt cs_low(); $($tail:tt)*) => {
///         mpsse!(@intern $passthru set_gpio_lower(0x0, 0xb); $($tail)*);
///     };
///     (@intern $passthru:tt cs_high(); $($tail:tt)*) => {
///         mpsse!(@intern $passthru set_gpio_lower(0x8, 0xb); $($tail)*);
///     };
///
///     // Hypothetical device-specific command. Leverages both user and libftd2xx commands.
///     (@intern $passthru:tt
///         const $idx_id:ident = command_42([$($data:expr),* $(,)*]);
///         $($tail:tt)*) => {
///         mpsse!(@intern $passthru
///             cs_low();
///             const $idx_id = clock_data(::libftd2xx::ClockData::MsbPosIn, [0x42, $($data,)*]);
///             cs_high();
///             $($tail)*);
///     };
///
///     // Everything else handled by libftd2xx crate implementation.
///     ($($tokens:tt)*) => {
///         ::libftd2xx::mpsse!($($tokens)*);
///     };
/// }
///
/// mpsse! {
///     const (DATA, DATA_READ_LEN) = {
///         wait_on_io_high();
///         const COMMAND_42_RESULT_RANGE = command_42([11, 22, 33]);
///         send_immediate();
///     };
/// }
/// ```

@newAM
Copy link
Member

newAM commented May 29, 2021

Should I go ahead and also add the following section to the macro doc?

Yup, looks good, though if it can run I would remove the no_run, just to make sure it does not hit any asserts or such.

@CirrusNeptune
Copy link
Contributor Author

CirrusNeptune commented May 29, 2021

Hmm, that CI failure is curious.

I get a passing result when running cargo test --all-features --target x86_64-unknown-linux-gnu on my local system with rustc 1.49.0 (e1884a8e3 2020-12-29) and rustc 1.54.0-nightly (5dc8789e3 2021-05-21).

I wonder if there's a macro shadowing bug in the CI compiler version?

EDIT: Yes, when I update to 1.52.1, there's a compiler regression.

@newAM
Copy link
Member

newAM commented May 29, 2021

Hmm, that CI failure is curious.

I get a passing result when running cargo test --all-features --target x86_64-unknown-linux-gnu on my local system with rustc 1.49.0 (e1884a8e3 2020-12-29) and rustc 1.54.0-nightly (5dc8789e3 2021-05-21).

I wonder if there's a macro shadowing bug in the CI compiler version?

Nice find!

I get it locally on 1.52.1 as well:

error: internal compiler error: failed to process buffered lint here
   --> tests/mpsse-macro.rs:228:45
    |
228 |               ::libftd2xx::mpsse!($($tokens)*);
    |                                               ^
...
232 | /     mpsse! {
233 | |         const (COMMAND_DATA, READ_LEN) = {
234 | |             wait_on_io_high();
235 | |             const COMMAND_42_RESULT_RANGE = command_42([11, 22, 33]);
236 | |             send_immediate();
237 | |         };
238 | |     }
    | |_____- in this macro invocation
    |
    = note: delayed at /rustc/9bc8c42bb2f19e745a63f3445f1ac248fb015e53/compiler/rustc_lint/src/early.rs:393:18
    = note: this error: internal compiler error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

thread 'rustc' panicked at 'no errors encountered even though `delay_span_bug` issued', compiler/rustc_errors/src/lib.rs:1013:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.52.1 (9bc8c42bb 2021-05-09) running on x86_64-unknown-linux-gnu

note: compiler flags: -C embed-bitcode=no -C debuginfo=2 -C incremental

@CirrusNeptune
Copy link
Contributor Author

It works in rustc 1.53.0-beta.3 (82b862164 2021-05-22). So it should be fine when the next stable release lands.

@CirrusNeptune
Copy link
Contributor Author

CirrusNeptune commented May 30, 2021

I have a better macro implementation that's more token hygenic. It does a better job of following the push-down accumulation pattern where the input and output tokens are isolated from each other by the syntax (rather than the shift-append approach). I also got rid of @intern because the syntax is so dang specific, it accomplishes the same purpose of avoiding accidental matches.

(<passthru>) {<input>} -> [<output>]

This guarantees that all input statements are consumed and the macro does not terminate early (without an error at least). It also makes it possible to shadow-override the recursion termination rule. I also put the id and read_len_id passthru tokens into their own token tree so an override can replace them with its own side-channel of tokens.

CirrusNeptune/libftd2xx-rs@mpsse-macro...CirrusNeptune:mpsse-macro-v2

I'm working on a proof of concept for the CC1101 RF module. The write_all and read_all is entirely contained within the macro and custom bitfield types are set in the variable bindings following the read.

I do not think it would be a good idea to provide read/write functionality directly in the libftd2xx macro because there are too many arbitrary details that are best left to the user (expansion scope, error handling, logging/tracing, data types, etc...). Instead maybe a sort of example shadow macro in the docs would be a good idea.

#[test]
fn all_commands_read() {
    let mut ftdi = MockFtdi::new();

    mpsse! {
        ftdi <=> {
            cs_high();
            let sres_status = sres();
            let sidle_status = sidle();
        };
    }
    assert_eq!(sres_status.chip_rdy_n(), false);
    assert_eq!(sres_status.state(), State::Idle);
    assert_eq!(sres_status.fifo_bytes_available(), 0);
    assert_eq!(sidle_status.chip_rdy_n(), false);
    assert_eq!(sidle_status.state(), State::Idle);
    assert_eq!(sidle_status.fifo_bytes_available(), 0);

    assert_eq!(ftdi.write_data, [
        MpsseCmd::SetDataBitsLowbyte as u8,
        0x8 as u8,
        0xb as u8,
        MpsseCmd::SetDataBitsLowbyte as u8,
        0x0 as u8,
        0xb as u8,
        ClockBits::MsbPosIn as u8,
        0x8 as u8,
        Command::SRES as u8,
        MpsseCmd::SetDataBitsLowbyte as u8,
        0x8 as u8,
        0xb as u8,
        MpsseCmd::SetDataBitsLowbyte as u8,
        0x0 as u8,
        0xb as u8,
        ClockBits::MsbPosIn as u8,
        0x8 as u8,
        Command::SIDLE as u8,
        MpsseCmd::SetDataBitsLowbyte as u8,
        0x8 as u8,
        0xb as u8,
        MpsseCmd::SendImmediate as u8,
    ]);
    assert_eq!(ftdi.read_len, 2);
}

Here are the non-generated parts of the macro if you're curious:
https://gist.github.com/CirrusNeptune/898881b0520183cc734af9cebb46effe

@CirrusNeptune
Copy link
Contributor Author

May I go ahead merge the v2 branch into this PR?

@newAM
Copy link
Member

newAM commented May 31, 2021

May I go ahead merge the v2 branch into this PR?

Yup, looks good to me!

@CirrusNeptune
Copy link
Contributor Author

I made a really dumb error and didn't subtract 1 from any of the data clocking implementations. Really should have tested with hardware rather than theoretical mock devices.

I'll push a fix in a bit here.

@CirrusNeptune CirrusNeptune requested a review from newAM June 1, 2021 15:24
@newAM
Copy link
Member

newAM commented Jun 2, 2021

Good catch, I should have tested on live hardware too 🙂

@CirrusNeptune
Copy link
Contributor Author

Rebased onto main

@newAM
Copy link
Member

newAM commented Jun 14, 2021

Looks like 1.53 is coming in two days so we can get this merged then 👍

Someone finally made a website to track this: https://www.whatrustisit.com/ its everything I ever wanted 😄

@newAM
Copy link
Member

newAM commented Jun 18, 2021

Rust 1.53 is out! I kicked off CI again, looks like everything is passing. I will test it out with a physical device this weekend and merge it in then.

@CirrusNeptune
Copy link
Contributor Author

Sounds good!

For reference it looks like this is the related issue: rust-lang/rust#84195

@newAM newAM merged commit 3d4cfa3 into ftdi-rs:main Jun 19, 2021
@CirrusNeptune CirrusNeptune deleted the mpsse-macro branch June 19, 2021 15:35
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.

None yet

2 participants