Add CORDIC Support #132

AdinAck · 2024-08-24T17:17:42Z

Create a new HAL component for using the CORDIC co-processor.

The interface permits configuration of all permutations of argument/result size and count, function, scale, and precision.

Configurations are represented by type-states and as such are compile-time validated.

Todo

I haven't added an interface for DMA (yet).

Example

As far as I can tell, examples are broken, so I only included a minimal example in the module-level docs. Here is that example:

#[entry]
fn main() -> ! {
    let dp = stm32::Peripherals::take().expect("cannot take peripherals");
    let pwr = dp.PWR.constrain().freeze();
    let mut rcc = dp.RCC.freeze(Config::hsi(), pwr);

    let cordic = dp
        .CORDIC
        .constrain(&mut rcc)
        .freeze::<Q15, Q31, SinCos, P60>(); // 16 bit arguments, 32 bit results, compute sine and cosine, 60 iterations

    cordic.start(I1F15::from_num(-0.25 /* -45 degreees */));

    let (sin, cos) = cordic.result();

    info!("sin: {}, cos: {}", sin.to_num::<f32>(), cos.to_num::<f32>());

    loop {}
}

usbalbin · 2024-08-24T20:24:48Z

As far as I can tell, examples are broken [...]

Anyone example in perticular? The goal is that they should all work AFAIK...

usbalbin · 2024-08-24T20:25:15Z

BTW, thanks a lot for your contribution! :)

AdinAck · 2024-08-24T20:32:05Z

As far as I can tell, examples are broken [...]

Anyone example in perticular? The goal is that they should all work AFAIK...

It must be something on my end, as CI of course is successfully building the examples... I'm running this command:

cargo build --examples --features stm32g431,log-rtt,defmt

And it's emitting large linker errors, I don't know why!

ERROR(cortex-m-rt): The interrupt vectors are missing.

BTW, thanks a lot for your contribution! :)

Of course!

usbalbin · 2024-08-24T20:56:51Z

I think #133 might be the cause of the errors you are seeing. The comparator related examples were likely broken for non G474 devices.

usbalbin · 2024-08-24T20:59:13Z

Does this work?

cargo build --example blinky --features stm32g431,log-rtt,defmt

AdinAck · 2024-08-24T20:59:52Z

Ahah! You're right! Nice, I'll add an example now. Thanks!

AdinAck · 2024-08-24T21:06:23Z

If you merge #133 I'll pull it from upstream and CI should pass :)

usbalbin · 2024-08-24T21:17:44Z

I have never used this peripheral so I do not know what the typical use case looks like.. But say you wanted to first run one function and then switch to another function.

// (some sort of pseudo rustish code)

fn do_math_stuff(cordic: &mut Cordic<...>) {
    cordic.do_sin_calc();
    
    cordic.do_sqrt_calc(); // <--- This would require to (atleast temporary) change type which is not allowed
    cordic.change_back_to_sin_mode();
}

I assume this would be not so simple to do as is. Again, not sure how this is typically used and if it at all is something we care about?

AdinAck · 2024-08-24T21:27:06Z

...say you wanted to first run one function and then switch to another function.

Since I felt it imperative for the peripheral state to be encoded in the type system, this can be done with the type system:

type CordicForSin = Cordic<_, _, Sin, _>;
type CordicForSqrt = Cordic<_, _, Sqrt, _>;

fn do_math_stuff(mut cordic: CordicForSin) -> CordicForSin { // cordic must be moved now
    cordic.start(_);
    let _ = cordic.result();

    let mut cordic: CordicForSqrt = cordic.freeze();
    
    cordic.start(_);
    let _ = cordic.result();

    cordic.freeze()
}

If something wants to change the resource then &mut is not enough.

I assume this would be not so simple to do as is. Again, not sure how this is typically used and if it at all is something we care about?

I did consider two options for enabling highly dynamic usage:

With allocation, one could accept an &mut to a Cordic<_, _, dyn func::State, _>.
Any type-states could be implemented for each configurable property.

Note: Unfortunately Rust doesn't have a way to specify ZeroSize like Sized, if it did, the dyn func::State would not require allocation since all type-states are ZSTs, which is why the creation of Any type-states is probably ideal.

My priority was maximizing safety and performance, but certainly either of these two options can be easily adopted later.

AdinAck · 2024-08-24T21:28:24Z

I don't know what itm or semihost are but it seems they change the println! macro, I'll just add a cfg...

usbalbin · 2024-08-25T09:31:49Z

Cargo.toml

+
+[[example]]
+name = "cordic"
+required-features = ["defmt"]


should not be needed after #134.

Suggested change

required-features = ["defmt"]

(Apparently the macro was only used and tested without formatting args.)

This command fails now:

cargo build --examples --features stm32g431,log-rtt

Looks like the log-rtt feature no longer enables the defmt feature?

usbalbin · 2024-08-25T09:44:57Z

src/cordic.rs

+    Func: func::State<Arg, Res>,
+    Prec: prec::State,
+{
+    rb: CORDIC,


Just a thought, what if rb was impl BorrowMut? Then the user could (optionally)

type CordicForSin = Cordic<_, _, Sin, _>; type CordicForSqrt = Cordic<_, _, Sqrt, _>; fn do_math_stuff(rb: &mut CORDIC) { { let mut cordic = rb.freeze_not_consuming(); // Find better name :) cordic.start(_); let _ = cordic.result(); } // <--- rb no longer borrowed by cordic, it is free to be reused let mut cordic: CordicForSqrt = rb.freeze(); cordic.start(_); let _ = cordic.result(); }

again, just a thought. Depending how the cordic is typically used, maybe not worth the extra generic arg.

and if this would be useful, I suppose it could be done as a future extension.

So, you nerdsniped me and I've been mulling this over since yesterday.

I just refactored the whole thing to be much stronger in terms of type relationships and enforcements.

As a result, now, adding this dynamic behavior is easy!

/// Traits and structures for dynamic function operation. pub mod dynamic { use super::{prec, signature, types, Cordic, State}; /// Any function can be invoked with this type-state. pub struct Any; /// A Cordic in dynamic mode. pub trait Mode<Arg, Res> where Arg: types::arg::State, Res: types::res::State, { /// Run a function with provided arguments and get the result. /// /// *Note: This employs the polling strategy. /// For less overhead, use static operations.* fn run<Func>(&mut self, args: Func::Arguments) -> Func::Results where Func: State<Arg, Res>; } impl<Arg, Res, Prec> Mode<Arg, Res> for Cordic<Arg, Res, Any, Prec> where Arg: types::arg::State, Res: types::res::State, Prec: prec::State, { fn run<Func>(&mut self, args: Func::Arguments) -> Func::Results where Func: State<Arg, Res>, { use signature::Property as _; self.apply_config::<Arg, Res, Func, Prec>(); args.write(&self.rb.wdata); self.when_ready(|cordic| Func::Results::read(&cordic.rb.rdata)) } } }

So your example would look like this:

fn do_math_stuff(cordic: &mut Cordic<_, _, Any, _>) { let sin = cordic.run::<Sin>(_); let sqrt = cordic.run::<Sqrt<N0>>(_); }

Cargo.toml

this will fail CI pending fix

src/cordic.rs

usbalbin

Thank you very much! This is awesome!

usbalbin · 2024-08-25T17:44:29Z

Is it ok if I merge now, or is there anything else you want to add?

AdinAck · 2024-08-25T17:46:05Z

Thanks, I'm pretty sure we're good 😅. I did start thinking about some minor organizational changes but they should be inconsequential.

Maybe let me sit on it for a day, if that's ok with you.

AdinAck · 2024-08-26T00:47:42Z

I decluttered the type signature from the fixed madness to just Q15 or Q31.

Something I realized that is really annoying me right now is that an enforcement strategy I came up with to further strengthen the system invariances seems to be impossible to implement!

Currently one could comment out a line of apply_config() and the program would still compile:

fn apply_config<NewArg, NewRes, NewFunc, NewPrec>(&mut self)
    where
        NewArg: types::arg::State,
        NewRes: types::res::State,
        NewFunc: func::Feature<NewArg, NewRes>,
        NewPrec: prec::State,
    {
        self.rb.csr.write(|w| {
            use func::reg_count::arg::State as _;
            use func::reg_count::res::State as _;
            use func::scale::State as _;

            NewArg::set(w.argsize());
            NewRes::set(w.ressize());
            NewPrec::set(w.precision());
            NewFunc::set(w.func());
            // NewFunc::NArgs::set(w.nargs()); whoopsies!
            NewFunc::NRes::set(w.nres());
            NewFunc::Scale::set(w.scale());

            w
        });
    }

Nothing about the signature requires all states to be set.

So I decided to make State::set() return the corresponding type-state, and apply_config() returns all type states, which could then be stored in the Cordic type instead of PhantomData (they're all ZSTs anyway so it would be the same).

fn apply_config<NewArg, NewRes, NewFunc, NewPrec>(&mut self) -> (NewArf, NewRes, ..)
    where
        NewArg: types::arg::State,
        NewRes: types::res::State,
        NewFunc: func::Feature<NewArg, NewRes>,
        NewPrec: prec::State,
    {
        let result;

        self.rb.csr.write(|w| {
            use func::reg_count::arg::State as _;
            use func::reg_count::res::State as _;
            use func::scale::State as _;
            result = (
                NewArg::set(w.argsize()),
                NewRes::set(w.ressize()),
                NewPrec::set(w.precision()),
                NewFunc::set(w.func()),
                NewFunc::NArgs::set(w.nargs()),
                NewFunc::NRes::set(w.nres()),
                NewFunc::Scale::set(w.scale()),
           );

            w
        });

        result
    }

But rust does not accept the deferred initialization of result in this context... for some reason. And the closure passed to write must return &mut W. So I think I'm out of options :(

Other than that I think everything is fine. I can continue to torment myself outside of this PR :)

usbalbin · 2024-08-31T09:03:12Z

Sorry for late answer

[..] But rust does not accept the deferred initialization of result in this context... for some reason. And the closure passed to write must return &mut W. So I think I'm out of options :( [..]

I guess rust does not know that write will actually call the closure so it can not guarantee that result will be initialized. I suppose you could (with some care) use something like MaybeUninit since we do know that write will call that closure.

Still, requires unsafe so not ideal... But I believe it would still enforce type safety, right?

fn apply_config<NewArg, NewRes, NewFunc, NewPrec>(&mut self) -> (NewArf, NewRes, ..)
    where
        NewArg: types::arg::State,
        NewRes: types::res::State,
        NewFunc: func::Feature<NewArg, NewRes>,
        NewPrec: prec::State,
    {
        let mut result = let mut x = MaybeUninit::uninit();

        self.rb.csr.write(|w| {
            use func::reg_count::arg::State as _;
            use func::reg_count::res::State as _;
            use func::scale::State as _;
            result.write(( // <--- Initialize result
                NewArg::set(w.argsize()),
                NewRes::set(w.ressize()),
                NewPrec::set(w.precision()),
                NewFunc::set(w.func()),
                NewFunc::NArgs::set(w.nargs()),
                NewFunc::NRes::set(w.nres()),
                NewFunc::Scale::set(w.scale()),
           ));

            w
        });

        // SAFETY: We trust `self.rb.csr.write` to have called the closure which initialized result
        unsafe { result.assume_init() };
    }

AdinAck · 2024-08-31T15:24:34Z

But I believe it would still enforce type safety, right?

~~It would certainly work but it wouldn't enforce anything. One could easily still comment out any call to set and it would compile and exhibit undefined behavior.~~

Actually no, I think it would work! It's a little less ergonomic but yes that is a way to do it.

I actually ended up getting this working by forking svd2rust and changing write and modify to support arbitrary return types. Then I rebuilt the PAC and went through all of the HAL and converted everything over to the new style (getters).

The result was perfection :)

pub fn freeze<NewArg, NewRes, NewPrec, NewOp>(self) -> Cordic<NewArg, NewRes, NewPrec, NewOp>
where
    NewArg: types::arg::State,
    NewRes: types::res::State,
    NewPrec: prec::State,
    NewOp: op::sealed::Feature,
    NewOp::NArgs<NewArg>: reg_count::arg::State,
    NewOp::NRes<NewRes>: reg_count::res::State,
    NewOp::Scale: scale::State,
    NewOp::Func: func::State,
{
    use func::State as _;
    use reg_count::arg::State as _;
    use reg_count::res::State as _;
    use scale::State as _;

    let config = self.rb.csr().modify(|_, w| Config {
        arg: NewArg::set(w.argsize()),
        res: NewRes::set(w.ressize()),
        nargs: NewOp::NArgs::set(w.nargs()),
        nres: NewOp::NRes::set(w.nres()),
        scale: NewOp::Scale::set(w.scale()),
        prec: NewPrec::set(w.precision()),
        func: NewOp::Func::set(w.func()),
    });

    Cordic {
        rb: self.rb,
        config,
    }
}

usbalbin · 2024-09-21T18:19:32Z

That is really nice!

What should we do in the mean time? :)

AdinAck · 2024-09-21T18:34:38Z

It works as is, there is some kind of bug somewhere because the resulting binary is smaller than doing the register writes manually. My finalized type-enforced version has no such bug. I am no longer working on the version in this PR, so it could be merged for now, and eventually when the PAC and svd2rust are updated, I can re-PR the final version.

usbalbin · 2024-09-21T18:48:01Z

Not sure I understand, but do you know if the MaybeUninit thing solves that?

AdinAck · 2024-09-21T18:55:04Z

Not sure I understand

I set up some tests where I configure the CORDIC with the PAC manually, and then do the same with the abstraction, and compare the resulting binaries. The version in this PR resulted in a different binary, so clearly it is not doing the exact same thing. With the version in my fork, this does not happen. I imagine there is a very subtle error somewhere that is not being caught by type-enforcement because that system is not in place.

do you know if the MaybeUninit thing solves that

I believe it could, I'm not sure, but I don't particularly want to pursue that avenue myself as I don't like the hacky and incomplete nature of it, sorry.

usbalbin · 2024-09-21T20:30:32Z

Not sure I understand

I set up some tests where I configure the CORDIC with the PAC manually, and then do the same with the abstraction, and compare the resulting binaries. The version in this PR resulted in a different binary, so clearly it is not doing the exact same thing. With the version in my fork, this does not happen. I imagine there is a very subtle error somewhere that is not being caught by type-enforcement because that system is not in place.

Thanks for the read! :)

do you know if the MaybeUninit thing solves that

I believe it could, I'm not sure, but I don't particularly want to pursue that avenue myself as I don't like the hacky and incomplete nature of it, sorry.

That's fair, no worries :)

I will merge this shortly. Again, thanks a lot!

AdinAck · 2024-09-21T22:58:05Z

Haha, of course!

AdinAck added 2 commits August 24, 2024 10:11

add CORDIC support

ec4e254

should fix CI failure

7a30931

add example

ba01283

Merge remote-tracking branch 'upstream/main' into add-cordic

049f660

require defmt for cordic example

4b60cda

usbalbin reviewed Aug 25, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

AdinAck added 3 commits August 25, 2024 08:48

Merge remote-tracking branch 'upstream/main' into add-cordic

3b746c9

put cordic stuff behind feature gate

37868a2

this will fail CI pending fix

significant refactor and dynamic mode

9f47dea

usbalbin reviewed Aug 25, 2024

View reviewed changes

src/cordic.rs Outdated Show resolved Hide resolved

usbalbin reviewed Aug 25, 2024

View reviewed changes

src/cordic.rs Outdated Show resolved Hide resolved

remove some erroneous whitespace, O -> OFFSET, more inline const exprs

21eab92

usbalbin approved these changes Aug 25, 2024

View reviewed changes

simplify type signature, distinguish state vs feature

545af80

usbalbin approved these changes Aug 31, 2024

View reviewed changes

usbalbin merged commit 906b83a into stm32-rs:main Sep 22, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CORDIC Support #132

Add CORDIC Support #132

AdinAck commented Aug 24, 2024

usbalbin commented Aug 24, 2024 •

edited

Loading

usbalbin commented Aug 24, 2024

AdinAck commented Aug 24, 2024

usbalbin commented Aug 24, 2024 •

edited

Loading

usbalbin commented Aug 24, 2024

AdinAck commented Aug 24, 2024

AdinAck commented Aug 24, 2024

usbalbin commented Aug 24, 2024 •

edited

Loading

AdinAck commented Aug 24, 2024 •

edited

Loading

AdinAck commented Aug 24, 2024

usbalbin Aug 25, 2024

AdinAck Aug 25, 2024

usbalbin Aug 25, 2024 •

edited

Loading

AdinAck Aug 25, 2024

usbalbin Aug 25, 2024

usbalbin left a comment

usbalbin commented Aug 25, 2024

AdinAck commented Aug 25, 2024

AdinAck commented Aug 26, 2024

usbalbin commented Aug 31, 2024 •

edited

Loading

AdinAck commented Aug 31, 2024 •

edited

Loading

usbalbin commented Sep 21, 2024

AdinAck commented Sep 21, 2024

usbalbin commented Sep 21, 2024

AdinAck commented Sep 21, 2024

usbalbin commented Sep 21, 2024

AdinAck commented Sep 21, 2024

Add CORDIC Support #132

Add CORDIC Support #132

Conversation

AdinAck commented Aug 24, 2024

Todo

Example

usbalbin commented Aug 24, 2024 • edited Loading

usbalbin commented Aug 24, 2024

AdinAck commented Aug 24, 2024

usbalbin commented Aug 24, 2024 • edited Loading

usbalbin commented Aug 24, 2024

AdinAck commented Aug 24, 2024

AdinAck commented Aug 24, 2024

usbalbin commented Aug 24, 2024 • edited Loading

AdinAck commented Aug 24, 2024 • edited Loading

AdinAck commented Aug 24, 2024

usbalbin Aug 25, 2024

Choose a reason for hiding this comment

AdinAck Aug 25, 2024

Choose a reason for hiding this comment

usbalbin Aug 25, 2024 • edited Loading

Choose a reason for hiding this comment

AdinAck Aug 25, 2024

Choose a reason for hiding this comment

usbalbin Aug 25, 2024

Choose a reason for hiding this comment

usbalbin left a comment

Choose a reason for hiding this comment

usbalbin commented Aug 25, 2024

AdinAck commented Aug 25, 2024

AdinAck commented Aug 26, 2024

usbalbin commented Aug 31, 2024 • edited Loading

AdinAck commented Aug 31, 2024 • edited Loading

usbalbin commented Sep 21, 2024

AdinAck commented Sep 21, 2024

usbalbin commented Sep 21, 2024

AdinAck commented Sep 21, 2024

usbalbin commented Sep 21, 2024

AdinAck commented Sep 21, 2024

usbalbin commented Aug 24, 2024 •

edited

Loading

usbalbin commented Aug 24, 2024 •

edited

Loading

usbalbin commented Aug 24, 2024 •

edited

Loading

AdinAck commented Aug 24, 2024 •

edited

Loading

usbalbin Aug 25, 2024 •

edited

Loading

usbalbin commented Aug 31, 2024 •

edited

Loading

AdinAck commented Aug 31, 2024 •

edited

Loading