Introduce implementation for EVSYS peripheral #882

rnd-ash · 2025-06-06T09:21:40Z

Summary

This is a finalized version of my earlier draft EVSYS peripheral implementation.

The EVSYS peripheral is a proprietary peripheral by ATMEL that allows all the peripherals to talk to one-another without the CPU. (This can be called sleepwalking)

This is a very powerful system, since it means you can chain Peripherals to trigger one-another and to then wake the CPU up once a chain of events has completed.

In this implementation, it is done using a Channel approach:

Create the Evsys peripheral (No clock is required)
Split the evsys peripheral into channels (Like EIC)
Pass a unregistered channel to a peripheral, which enables the generator and event bit. This then returns a GenReady evsys channel (Generator ready)
Pass this generator ready channel into a consuming peripheral in order to complete the chain between 2 peripherals.

To free an evsys channel, the process must be done in reverse (Unwire the consumer peripheral, and then the generator).

I have modified the enable_event functions for EIC such that they now do the generator wiring process, and also have provided a disable_event function for EIC to unregister the generator.

There is no consumer peripheral at this time. But, I could add a pulse counter peripheral (Using the TCCs to consume events to count pulses.)

jbeaurivage

I have a few minor comments, but overall I think this is quite nice, and has a pretty simple and readable API+implementation. One thing that we have discussed over Matrix:

What I'm somewhat worried about is the proliferation of of generic type params in every peripheral it touches... It's how we've done it for DMAC et al, I'm just worried that as all peripherals become increasingly interconnected, we'll keep piling on type parameters. Not that it's necessarily wrong, we just need to approach it carefully. Perhaps that discussion should be had in a separate issue though.

jbeaurivage · 2025-06-11T18:45:54Z

hal/src/peripherals/eic/d11/pin.rs

        self.chan.with_disable(|e| {
            e.evctrl()
                .modify(|_, w| unsafe { w.bits(1 << P::ChId::ID) });
        });
+        let hooked_channel = channel.register_generator((P::ChId::ID as u8) + 0x0C);


Maybe break this 0x0C constant out in a const, and document the table it comes from?

jbeaurivage · 2025-06-11T18:46:04Z

hal/src/peripherals/eic/d5x/pin.rs

        self.chan.with_disable(|e| {
            e.evctrl()
                .modify(|_, w| unsafe { w.bits(1 << P::ChId::ID) });
        });
+        let hooked_channel = channel.register_generator((P::ChId::ID as u8) + 0x12);


jbeaurivage · 2025-06-11T18:50:06Z

hal/src/peripherals/evsys.rs

+}
+
+pub struct EvSysChannel<Id: ChId, S: Status> {
+    evsys: core::mem::ManuallyDrop<Evsys>,


Does pac::Evsys have special Drop semantics to require the use of ManuallyDrop?

Actually, come to think of it, I wonder if the channels (or even the entire EVSYS controller) should get disconnected when dropped. It's not a memory safety issue like with DMAC, more a matter of managing what the expected behavior should be.
I suppose most people would view the EVSYS as more of a "set and forget" thing, where they would expect it to keep working forever unless they manually stop it.

the manuallyDrop usage here is due to me copying the EIC implementation for how channels were done. I think it is needed however to keep the borrow checker happy.

Oh, right. I think this is an artifact of a previous design in EIC where the Channel struct held something else than the PAC EIC struct. It doesn't do anything here (or in EIC), but it doesn't hurt either. In fact, it's a little defensive, in case we choose to change the Channel struct to hold something else than the PAC struct eventually. Would you mind adding comments to that effect in the EVSYS and EIC structs just for future reference?

I'd rather not have code that isn't needed - it makes code harder to read. In that hypothetical where we need the ManuallyDrop, is there a reason we couldn't add it back in then?

jbeaurivage · 2025-06-11T18:51:10Z

hal/src/peripherals/evsys.rs

+    generator_id: u8,
+}
+
+// Create a new Uninitialized channel


It's nice to leave those as doc comments even though it's not part of the public API, if only to be able to view the docs from an IDE :)

jbeaurivage · 2025-06-11T18:57:24Z

hal/src/peripherals/evsys.rs

+#[hal_cfg("evsys-d5x")]
+impl EvSysController {
+    pub fn new(mclk: &mut Mclk, evsys: crate::pac::Evsys) -> Self {
+        mclk.apbbmask().write(|w| w.evsys_().set_bit()); // Enable EVSYS clock


Would we rather take a token from the clock::v2 system instead?

Good point, i forget that that is an option with V2

jbeaurivage · 2025-06-11T18:58:25Z

hal/src/peripherals/evsys.rs

+#[hal_cfg("evsys-d11")]
+macro_rules! with_num_channels {
+    ($some_macro:ident) => {
+        $some_macro! {6}
+    };
+}
+
+#[hal_cfg(any("evsys-d21"))]
+macro_rules! with_num_channels {
+    ($some_macro:ident) => {
+        $some_macro! {12}
+    };
+}
+
+#[hal_cfg(any("evsys-d5x"))]
+macro_rules! with_num_channels {
+    ($some_macro:ident) => {
+        $some_macro! {32}
+    };
+}
+
+macro_rules! get {
+    ($literal:literal) => {
+        $literal
+    };
+}
+
+pub const NUM_CHANNELS: usize = with_num_channels!(get);
+
+macro_rules! define_channels_struct {
+    ($num_channels:literal) => {
+        seq!(N in 0..$num_channels {
+            #(
+                /// Type alias for a channel number
+                pub enum Ch~N {}
+
+                impl ChId for Ch~N {
+                    const ID: usize = N;
+                }
+            )*
+
+            /// Struct generating individual handles to each EVSYS channel
+            pub struct Channels(
+                #(
+                    pub EvSysChannel<Ch~N, Uninitialized>,
+                )*
+            );
+        });
+    };
+}
+with_num_channels!(define_channels_struct);
+
+macro_rules! define_split {
+    ($num_channels:literal) => {
+        seq!(N in 0..$num_channels {
+            /// Split the EVSYS into individual channels.
+            #[inline]
+            pub fn split(self) -> Channels {
+                Channels(
+                    #(
+                       unsafe { new_evsys_channel(core::ptr::read(&self.evsys as *const _)) },
+                    )*
+                )
+            }
+
+        });
+    };
+}


This pattern is starting to repeat itself quite often in peripheral implementations, I wonder if we should make it reusable somehow.

ianrrees

Looks like a good start, I share the concern that @jbeaurivage expressed about proliferation of type parameters. As we've discussed before, the event system doesn't seem to be very widely used, so if introducing support for it makes the API for everything else more complicated then that's a tough sell.

To make this a little clearer, it might be worth adding event system support to another peripheral, in a separate commit but in this same PR. That way, we could have a working example of what the changes to other peripherals will look like, isolated from the EVSYS peripheral support (EIC seems to be a special case in that it already uses EVSYS).

If that event system support for another peripheral does require adding a type parameter, then it might be worth considering an enum r/t type parameter for the channel ID.

ianrrees · 2025-06-11T23:23:07Z

hal/src/peripherals/evsys.rs

+//! 2. Split the controller into a tuple of its channels, using [`EvSysController::split`]
+//! 3. Pass a channel into a generator peripheral to enable it to output to the provided channel
+//! 4. Pass the channel into the receiving peripheral to complete wiring the peripheral up.
+//!


I'd like to see example code here, along the lines of what we've got in the SERCOM etc.

ianrrees · 2025-06-11T23:26:09Z

hal/src/peripherals/evsys.rs

+//! * SAMD11: 6 channels
+//!
+//! The channels destinations are configured using a multiplexor register, which directs events to
+//! a destination peripherals. Destination peripherals can support 1 or more supported path modes:


Without having read the datasheet about the event system, this doesn't tell me what path modes do; I'm wondering "synchronous with what?".

ianrrees · 2025-06-11T23:27:41Z

hal/src/peripherals/evsys.rs

+}
+
+pub struct EvSysChannel<Id: ChId, S: Status> {
+    evsys: core::mem::ManuallyDrop<Evsys>,


I'd rather not have code that isn't needed - it makes code harder to read. In that hypothetical where we need the ManuallyDrop, is there a reason we couldn't add it back in then?

ianrrees · 2025-06-14T23:25:11Z

An idea of how we might implement Event System without type system noise; this is just a half-baked thought, and I haven't thoroughly studied the event system, so please poke holes in it. One assumption I make is that there is no problem with initializing two peripherals, connecting them via the event system, then disabling one/both - I presume that in this scenario the events will simply stop originating and/or being handled.

Add new traits like this, which we can implement on the various peripherals:

/// EventChannel is maybe just a channel number
type EventChannel = u8;

/// Implemented by peripherals that can source events
trait EventSource {
    /// Enable `self` as an event source on `channel`
    fn start_event_source(&mut self, channel: EventChannel);

    /// Returns the EventChannel that `self` sources events to
    fn get_event_source_channel(&self) -> Option<EventChannel>;

    /// Disables `self` as an event source
    fn stop_event_source(&mut self);
}

trait EventSink {
    /// Enable receiving events from `channel`
    fn start_event_sink(&mut self, channel: EventChannel);

    /// Disable receiving events from `channel`
    fn stop_event_sink(&mut self, channel: EventChannel);
}

Then, the EVSYS object can use those traits to manage the connections.

struct EventSystem {
    /// Refcount of the number of sinks using each channel
    sinks_of_channel: [u8; NUM_CHANNELS];
}

impl <F: EventSource, T: EventSink> EventSystem {
    fn next_free_channel(&self) -> Option<EventChannel> {
        for (i, count) in sinks_of_channel.iter().enumerate() {
            if *count == 0 {
                return Some(i);
            }
        }
        None
    }

    pub fn connect(&mut self, source: F, sink: T) -> Result<()> {
        let channel = source.get_event_source_channel().or_else(|| {
            self.next_free_channel().and_then(|channel| {
                source.start_event_source(channel);
                Some(channel)
            })
        }).ok_or(Error::NoChannel)?;

        self.sinks_of_channel[channel] += 1;
        
        sink.start_event_sink(channel);

        // Do whatever EVSYS work is requied
        
        Ok(())
    }

    pub fn disconnect(&mut self, source: F, sink: T) {
        let channel = source
            .get_event_source_channel()
            .ok_or(Error::NoChannel)?;

        if self.sinks_of_channel[channel] == 1 {
            source.stop_event_source();
        }
        self.sinks_of_channel -= 1;

        sink.stop_event_sink(channel);

        // Do whatever EVSYS work is requied
    }
}

… clock

rnd-ash · 2025-06-28T07:26:08Z

@ianrrees I re-worked the API, such that it still uses compile-time Generic enforcement, but without any pollution to the peripherals themselves. For a peripheral to be used as a source for Evsys now just involves implementing the EvSysGenerator trait (Which just has a Const value holding the EVGENID

rnd-ash added 3 commits June 6, 2025 09:45

Add evsys peripheral

a14c825

Add EIC wiring to evsys

f9aae9b

Restrict enable/disable event depending on evsys channel generic

c743564

jbeaurivage reviewed Jun 11, 2025

View reviewed changes

ianrrees reviewed Jun 11, 2025

View reviewed changes

Reduce the number of generics involved with using EVSYS

b6d0ac2

rnd-ash added 5 commits June 15, 2025 11:42

Bug fix EIC

7335d37

Clock V1 (D5x) - Allow to convert from a Pclk reference to Peripheral…

9966b47

… clock

Doc updates for EIC and pulse counter

567768b

Note the maximum pulse frequency calculation

f9e1fc5

Merge remote-tracking branch 'origin/master' into feat-evsys

fb545e2

rnd-ash mentioned this pull request Jun 28, 2025

clock::v2 API migration discussion and tracking #912

Open

Introduce implementation for EVSYS peripheral #882

Are you sure you want to change the base?

Introduce implementation for EVSYS peripheral #882

Uh oh!

Conversation

rnd-ash commented Jun 6, 2025

Summary

Uh oh!

jbeaurivage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianrrees left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianrrees commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnd-ash commented Jun 28, 2025

Uh oh!

Uh oh!

ianrrees left a comment •

edited

Loading

ianrrees commented Jun 14, 2025 •

edited

Loading