Added interrupt driven UARTE (open for feedback, do not merge yet)

[korken89]

Hi nrf team,

We have added the interrupt driven code from the RTFM workshop @ Oxidize and would like feedback.

Purpouse:

1. Showcase how to design interrupt driven HALs
2. Showcase the interoperability with RTFM (example will be added)
3. Showcase that ownership is well defined at all times, even in interrupt driven designs

Please note:

1. The DMAPool is fixed to 16 byte size, which we should discuss on. Currently this means that only full 16 byte chunks can be sent or received, which we should look into fixing.
2. The transmit buffer is currently fixed at 4 chunks maximum, this means that the user can enqueue 4 outstanding DMA buffers
3. There is an issue with min-const-fn in heapless, but we are currently looking into that

Future extensions:

• Allow variable size on sent packages (rather than full DMA chunks)
• Add a timeout for receive, allowing for receiving non-full chunks
• Look into the "short-cut" feature of the UARTE, we tried it - but did not get it working for now

Best Regards
Emil Fresk & Per Lindgren (@perlindgren)

[SamP20]

Would you mind if I suggested an alternative solution. Instead of using a DMAPool I'm wondering whether start_read and start_write could take a reference to a u8 slice and return a Read/WriteHandle with the same lifetime. That handle can then be used to check the status of the DMA transfer. If the DMA is still going when the handle is dropped then it is cancelled, allowing the buffer to be safety used again.

I'm not sure which solution is best, but I'd be happy to try implementing it so we can compare.

[korken89]

Not quite sure what difference you are suggesting, this sounds exactly like what we are doing with the Box<DMAPool>, as this (in practice) gives us an owned slice.

Or did you have something different in mind?
Thanks for the feedback!

[SamP20]

I've written up a boilerplate implementation as it's probably easier to explain than with words SamP20@54bb0c1

The main changes are that UarteRX and UarteTX do not take ownership of the DMAPool, instead that's the job of the begin_read and begin_write methods which return an Option<ReadHandle> and Option<WriteHandle> respectively. These are Options since for the RX we can only have two buffers at once, and for TX double buffering doesn't even gain us anything compared to just using interrupts due to the lack of a ENDTX_STARTTX short.

Edit: I haven't included the details of how the interrupts work with this, although for RX this would involve checking the ENDRX event, and if set then disabling the ENDRX_STARTRX short, and updating any information to keep track of which ReadHandle is active.

[korken89]

@SamP20 I had a look at the implementation. The comment I have is that now you have pushed the ownership issue to the user by taking a reference to the memory, without any significant improvement in performance (the Box<DMAPool> was specifically chosen to not have the ownership issue on the user side). In the end the user must have some pool of memory, which persists between calls, to take references from.

Do you have an example use-case where manually handling the ownership is advantageous or, when automatically handling ownership is an issue?
I'd like to better understand the use-case you are working towards, maybe there is a better method that we do not see. :)

[SamP20]

After looking through your implementation some more, I'm starting to see the advantages of your solution, and to be honest I think I had completely the wrong idea with my approach, so sorry about that! That being said there is a use case I'd like to support, however I don't know if it's mutually exclusive with what you've imeplemented.

Firstly I'd like to recognise the merits of your solution when paired with RTFM. It returns the data in the UART0 interrupt as a Boxed object which can then be easily sent to other tasks for processing. As soon as the Box is dropped, the memory is recycled :)

I'm working on a thread scheduling library which takes a different approach. Without going into too much detail I can spawn multiple threads that can take ownership of variables from their surrounding context (Almost identical to std::thread). The threads can yield control back to the main thread by calling yieldk().

With that in mind I'd like to be able to do something similar to the pseudocode below (the inner read loop would be wrapped up in a macro):

let uartbuffer = [0u8; 128];
let uart = Uarte::new(&mut uartbuffer, peripherals.UARTE);

let thread = Thread::new(stack, move || {
    loop {
        let data = loop { // Keep looping until we get some data
            match uart.try_read() {
                Ok(data) => break Ok(data),
                nb::Error::Other(e) => break Err(e),
                nb::Error::WouldBlock => {
                    // Schedule the thread for wakeup
                    UART_BLOCKING_THREAD = Thread::get_current();
                    // Yield control back to main
                    yieldk();
                }
            }
        }
        // Do stuff with data
    }
});

run_threads(& mut[thread]);

In the above implementation the data handle would only be allowed to live as long as the uart itself otherwise you risk breaking Rust's safety. That being said, in RTFM I believe you would make the uart 'static which means that the data would be static too and you can still pass it to wherever you need.

In an interrupt handler I woud like something similar to this:

// Ensure we are the only interrupt with a handler
static mut handler = Uarte<UART0>::take_interrupt_handler().unwrap();

if handler.data_received() {
    UART_BLOCKING_THREAD.wakeup();
}

It may be this this usage is too far from the current implementation, and that would be understandable, however the solution in this PR currently doesn't follow Rust's ideal of zero cost abstractions due to the memory overhead of the DMAPool. Edit: an easy fix could be to feature gate it.

I hope that wasn't too much to take in, and let me know if you need me to clarify anything. I do hope there's a solution that caters for the best of both worlds :)

[perlindgren]

Hi Sam,
I think your approach could be paired with our PR.

1. RTFM is a resource manager and scheduler on top of which you can implement further abstractions (like the thread like API you show).
2. You can implement your user thread scheduler in "idle", and enjoy the merits of having RTFM handling the real-time tasks and interrupts.
3. Ownership can be moved from the interrupt handler to "idle" where your scheduler dispatch the cooperative tasks.
4. If you want, you can use RTFM tasks at higher priority (i.e., a thread scheduler for each priority level). The only thing to keep in mind is that each thread must run into a yield at some point, like this uart.try_read, should if no data is present automatically yield the thread, in your case its the behaviour you encoded, so its fine. This way your cooperative scheduler will automatically be a preemptive scheduler. You can use the same approach to communicate messages/signals in between your tasks as well. At RTFM level the messages can be an enum of all messages in the system. If that would be too much OH, then you can setup a heapless queue/split it and have a channel directly between your threads, and just use a signal to ping/wake the receiver.
5. If you want the interrupt handler (RTFM-task) to be fully ignorant to the the set of threads, it should post the message to the highest priority thread handler. That would dispatch any/the listener if there is no listener at that priority it passes the message downwards to the next priority scheduler.
6. We made some experiments using generators to achieve this statically (cooperative multi tasking). Ideally you pass the message into the generator on resume. The problem is that generators (at that time) could not take values passed into them (if I recall correctly), so we decided to postpone the implementation awaiting better generators in Rust. Thing might have improved since then. Also if you do it without generators (by hand) I think you can also achieve what you want, i.e., a traditional thread like API, on top of a real-time resource manager/scheduler.

What do you think?
/Per

[perlindgren]

On a side note. If doing a threading API, make sure never to involve any spinning Mutex behavior, its ok to try_lock, and yield on fail. Bare in mind that RTFM resource lock is just claiming the resource, it never needs to wait for it (its guaranteed to succeed). Hence you never run-into the dreaded dead-locking associated to Mutex:es. In fact the try_read (as sketched above) will never fail, your cooperative thread will be woke by a message (which might be a transmit error), but never of WouldBlock type, which reflects the reactive nature of the system, which is inherent to the embedded systems running on top of interrupt driven hardware.

Best
Per

[korken89]

@SamP20 Thanks for giving insight into your use-case! I'd like to propose a different way of performing "yield" like actions without actually needing yeild.

Let's say you have 3 processing steps that are all part of the same interrupt handler, and yielding is needed:

#[interrupt]
fn handler() {
    processing1();
    yield();

    processing2();
    yield();

    processing3();
}

This can be mapped to RTFM (or other event based systems) by having a state machine as:

enum States {
    Processing1,
    Processing2,
    Processing3,
}

#[interrupt]
fn handler_sm() {
    static mut state: States = States::Processing1;

    match state {
        States::Processing1 => {
            processing1();
            state = States::Processing2;
        }

        States::Processing2 => {
            processing2();
            state = States::Processing3;
        }

        States::Processing3 => {
            processing3();
            state = States::Processing1;
        }
    }
}

However, this is quite verbose to always write over and over again, so I have been working on a State Machine Language DSL, in which the following could be done to get the same behavior:

use sml::statemachine;

statemachine!(
    *State1 + ProcessingEvent / processing1 = State2,
    State2 + ProcessingEvent / processing2 = State3,
    State3 + ProcessingEvent / processing3 = State1,
);

#[interrupt]
fn handler_sml() {
    static mut sm: StateMachine = StateMachine::new();

    sm.process_event(States::ProcessingEvent);
}

How do you see on this pattern, what are your thoughts / worries? With this we can always stay within the strong guarantees of RTFM (or similar) and still get yields.

---

On this note (@SamP20):

It may be this this usage is too far from the current implementation, and that would be understandable, however the solution in this PR currently doesn't follow Rust's ideal of zero cost abstractions due to the memory overhead of the DMAPool. Edit: an easy fix could be to feature gate it.

The meaning of zero cost is that there is no way to do it better by hand (and not that the operation is free), and in the end you must solve the ownership problem, together with handling of "chunks" with correct owner ship, where pool internally uses a Treiber stack (which is one of the best available). So I would argue that this is, if not zero-cost - very close to zero-cost.

Or maybe I misunderstood your thought about this? :) I'm very open to discussing this!

[SamP20]

Thanks for the state machine idea. That sounds almost like async/await and Future behind the scenes. It does have an overhead compared to storing a single pointer to the next instruction, but you don't have to worry about allocating a separate stack. That being said I can now see how RTFM could work with my threading library:

#[task(resources = [THREAD1])]
fn run_thread1() {
    THREAD1.switch_to(); // Runs the thread until it yields
}

#[task(resources = [THREAD2])]
fn run_thread2() {
    THREAD2.switch_to();
}

How does the UARTE fit into your state machine model? Also how would you manage reading data from different peripherals in different states? For example in one state you might be waiting on UART0 data, but in a later state you might be waiting on the i2c bus.

Whan I talk about zero cost, I mean if I import the nrf52-hal crate but decide not to use the UARTE, then it shouldn't consume any extra memory in the final code. If any static pools are required then in my opinion it's the end user that should create them.

@perlindgren I already have mutexes yielding on failure. They also keep a list of waiting threads so they can be woken up once the mutex is released. Take a look here if you're curious. Also I'm unable to have threads preempt eachother at the moment due to how they're implemented. The ARM cortex architecture has two stack pointers, MSP and PSP. Most of the time the MSP is used, but I have the Thread::switch_to and yieldk methods trigger the SVCall interrupt which switches between the MSP and PSP.

[perlindgren]

@SamP20, some update are on the way.

1. The size of the store for outgoing buffers is now a type parameter. (I upped the changes, bot sure they are in the PR.)

2. I'm looking to parametrise the DMA_SIZE buffer (used by the pool)!. There are several options here, we could use a features flag, to chose in between a predefined set of DMA_SIZEs, 4,8,16,32,64,128,256 (or maybe 255 is the max if 0 is not used to define a size of 256 by the HW, haven't tried...). The other option is to use a crate for storing the constant, and let the user do a patch in the Cargo.tom, to override this setting by a local crate giving the setting. The pool! and all associated functionality could be hidden behind a feature flag, if the application does not use the pool! approach. So no OH in that case.

3. Regarding cooperative multit-hreading vs threads (with separate stacks), the generator approach will handle the stack for you, so you don't need to have to manage context switch manually. This will work primitively (and preemptively) out the box, as describe above.

4. Using RTFM + generators as the underlying infrastructure will allow you to implement cooperative (within a priority) and preemptive (in between different priorities) multi-tasking ultimately in safe Rust. This is a huge step towards reliability guarantees.

5. We have initiated an experiment to implement a "Tock" like API, sandboxing user applications (written in C, or whatever), from each other and the kernel code. We came upon the idea at the Oxidize conference (results of discussions with the Helium team). The approach is tentatively coined DetOx [de:tocks], and currently is just a PoC to showcase the MSP/PSP handling (switching in between "sandboxes"), but does not yet setup the MPU etc. (so its not yet a real sandbox). User applications can call into the kernel through a SVCall, and the SVCall is used to switched stacks back into the user space. The code needs some cleaning up but the approach works. https://github.com/lthiery/rtfm_workshop/tree/stack-swap (for the one with the call-back from user space, I did not personally test it yet), https://github.com/lthiery/rtfm_workshop/tree/sandbox (for the first attempt to switch into user space). The code has not been vetted, so there may be bugs ;) In any case if you are interested in working with DetOx, we can see how to best progress this.

Best regards
Per

[Yatekii]

OW this all sounds great =) Looking forward to merge this ;)

[perlindgren]

I added tentative support for setting DMA_SIZE from the users side. The pool! needs to be in the driver (for the driver to see it). In the user side you give a feature to set size.

[dependencies.nrf52832-hal]
version          = "0.8.1"
default-features = false
#features = [ "DMA_SIZE_4", "xxAA-package", "rt" ] # tested ok
#features = [ "DMA_SIZE_8", "xxAA-package", "rt" ]
#features = [ "DMA_SIZE_16", "xxAA-package", "rt" ] # tested ok
#features = [ "DMA_SIZE_32", "xxAA-package", "rt" ]
#features = [ "DMA_SIZE_64", "xxAA-package", "rt" ]
#features = [ "DMA_SIZE_128", "xxAA-package", "rt" ] # tested ok
#features = [ "DMA_SIZE_256", "xxAA-package", "rt" ] # tested error, causes OOM

features = [ "xxAA-package", "rt" ] # DMA_SIZE_16 per default # tested ok

So if you don't give an explicit DMA_SIZE it will default to 16.
I have prepared for a gate to enable/disable the pool!, but not yet implemented it. Whats your suggestion?

// The DMA implementation shuld be hidden behind a feature POOL
// This likely requires a mod {} around the related code
// or the non-ergonomic repetition of the gate.
// Is there a better solution?
//
// The reason to have a POOL gate is that we don't want the POOL
// to cause memory OH if not used by the application

pool!(DMAPool: [u8; DMA_SIZE]);

An example using the DMA pool, can be found in
git@github.com:korken89/rtfm_workshop.git
under the branch, newhal.

It requires some local crates in the parent directory (or you may change the Cargo.toml patches).

[patch.crates-io]
nrf52-hal-common        = { path = '../nrf52-hal/nrf52-hal-common' }
nrf52832-hal            = { path = '../nrf52-hal/nrf52832-hal' }
cortex-m-semihosting    = { path = "./cortex-m-semihosting"}
dwm1001                 = { path = "../rust-dwm1001"}

The nrf related points to the PR.
The dwm1001 patch points to https://github.com/korken89/rust-dwm1001, a version of the dwm1001 compatible to the latest nrf hal (not related to this PR directly, but is required for this code to work).

The pool example in rtfm_workshop, opens a uarte and echoes the received buffer at 115200 8N1. The effect of DMA_SIZE can be observed by first sending DMA_SIZE - 1 bytes, (nothing should be echoed), and then later a single byte. At that point the whole buffer will be received and echoed. Try with DMA_SIZE_4 for simplicity. (Its been tested and seems to work for 4, 16, 128, but not 256, in that case it runs into an OOM, I haven't got around to finding out why, gdb bt did not work).

Best regards,
Per

[Yatekii]

Thanks for the heaps of work, I am really excited to see this coming!

What I wonder about is, how you would handle this with the nRF52840 that has DMA_SIZE = 2^16.
I would hate it to be limited to 256 byte chunks with that chip too. I guess once const generics hit his is no problem anymore anyways, right?

[perlindgren]

@Yatekii, well the approach I took for now was an enumeration of DMA_SIZE_xx, but that is clearly not the way to go if we want it to be fully generic. We will see what const generics might bring to the table (hopefully it will be of help). In the meantime, we can take a detour through the module system, and use a crate to store the default size, and just override the crate by a patch section in the user Cargo.toml. Its quite straightforward, and more generic than the "feature" enumeration.

Right now I'm hunting a bug to be able to run everything on stable, in the worst case we just have to wait out the const_fn, which is due to land sometimes during the summer.

Best regards
Per

[Yatekii]

Sounds good to me! Thanks!

[perlindgren]

Seems like the Maybeuninit is now in master, so next beta will have it and after that stable. Thus, the PR will be mergable and useful as a reference as how to write interrupt driven HALs. Any further suggestions/improvements are of course welcome.

[korken89]

The code is now updated with byte timeout functionality and being able to read and write fewer bytes than a full DMA package!
For now the timeout is set to 10 ms, this should maybe be settable. If someone can have a look and give feedback it would be awesome.

cc @perlindgren @jacobrosenthal @Yatekii @jamesmunns

[korken89]

The public interface has been simplified and more checks have been added as well.

[jacobrosenthal]

ah, timeout feels nice

Fix for printer, maybe something like

        // enqueue a test message
        // let mut node = UARTEDMAPoolNode::new();
        // node.write(&[95, 95, 95, 95]);
        // let b = UARTEDMAPool::alloc()
        //     .unwrap()
        //     .init(node);
        // let _ = resources.PRODUCER.enqueue(b);

also thoughts on impl debug for the hprintln were debug for MaybeUninit doesnt work currently?

    = help: the trait `core::fmt::Debug` is not implemented for `core::mem::MaybeUninit<[u8; 16]>`
    = note: required because of the requirements on the impl of `core::fmt::Debug` for `&core::mem::MaybeUninit<[u8; 16]>`
    = note: required for the cast to the object type `dyn core::fmt::Debug`

[korken89]

Thanks for the feedback @jacobrosenthal !
I have updated the code with your suggestions.

[korken89]

Interesting error when merging with master, it seems that the nRF9160 core does not have support for the pool! from heapless. I have to take a closer look.

[jamesmunns]

Does heapless not support thumbv8* yet?

[korken89]

It seems so, yes - from a the Cortex core perspective it should be fine. I think the feature flag check in heapless just needs updating.

[korken89]

@jamesmunns I made a PR for it here: rust-embedded/heapless#113

[jacobrosenthal]

Probably outside of the scope of this pr..

but any thoughts on avoiding multiple buffers by writing directly into the dmapoolnode

TLDR I thin this is the old rolling your own core::io problem right?

Im looking at cbor which recently got no_std support.

This is the naive mutiple copy scenario from the docs, (havent tested beyond that it compiles)

	use serde::Serialize;
	use serde_cbor::Serializer;
	use serde_cbor::ser::SliceWrite;

	let mut buf = [0u8; 100];
	let writer = SliceWrite::new(&mut buf[..]);
	let mut ser = Serializer::new(writer);
	let user = User {
	    user_id: 42,
	    password_hash: [1, 2, 3, 4],
	};
	user.serialize(&mut ser).unwrap();
	let writer = ser.into_inner();
	let size = writer.bytes_written();

	let mut node = UarteDMAPoolNode::new();
	node.write(&buf[0..size]);
	let b = UarteDMAPool::alloc().unwrap().init(node);

	// just do the buffer dance without copying
	resources.PRODUCER.enqueue(b).unwrap();
	rtfm::pend(interrupt::UARTE0_UART0);

You can see it news up a SliceWriter with a &mut [u8] https://github.com/pyfisch/cbor/blob/0dfd50959a53dbf1a3d1ac55cc1735b104956c8f/src/write.rs#L139 but our dmapoolnode doesnt degrade to that . It also implements their fake writer trait only supporting write_all fn https://github.com/pyfisch/cbor/blob/0dfd50959a53dbf1a3d1ac55cc1735b104956c8f/src/write.rs#L24

and your dmapoolnode exposes a write trait

Anyone heard anything on core::io Should dmapoolnode implement something here? or should I bother cbor? Or am I missing a simple solution

[korken89]

Good feedback, thanks!
What we can do is implement as_slice traits for the UarteDMAPoolNode to allow it to be viewed directly as a slice. This way you do not need to first have a &[u8] to give to the write function.

This way you can short circuit, and not need the intermediary buffer as long as the thing before support to directly pump data to a &mut [u8].
What do you think?

[jacobrosenthal]

yeah I think that would do it for this case. Id offer, but Im not sure how that works with Maybeuninit

[korken89]

Thanks for finding @jacobrosenthal I'll change to that!

Also I just rebased all this on the master to be in sync again.

[korken89]

So this has been working for a long while now, how does @nrf-rs/all feel about this? Time to start bikesheding names? Or are there questions/thoughts that need to be handled?

[hannobraun]

Did a rough review, left some comments. Also needs a rebase.

[hannobraun]

Why make the version requirement less specific? I think it can be useful to see what the latest version was, when a dependency was added or last updated.

I also think being less specific can make breakage more likely in some scenarios, although this is unlikely to be the case here.

[hannobraun]

Why the >= requirement? Won't this include 0.6 and later versions, which may break this code?

[hannobraun]

I know everyone's tired of hearing it, but this sucks. While tool-supported consistent formatting has its merits, that whole notion, that it needs to be applied universally, and that everyone needs to agree, is just plain wrong.

I don't care about consistency in formatting. I care about readability. This is a clear example where readability suffers, as is every other svd2rust-generated API. Since using svd2rust APIs is the whole point of this project, using rustfmt here is simply a bad idea.

I won't block merging this PR over this, but I'd really appreciate it if not every pull request came with a million formatting changes to unrelated code. This just makes the diffs harder to review and in many cases (like this one) it makes the code plain worse.

[therealprof]

Actually I have to disagree. I find the official style much more readable than the previous version.

[hannobraun]

I have to say I'm baffled about this comment. Those svd2rust function calls always come in pairs, so I figured it was self-evident that any formatting choice that makes it easy to immediately recognize what those pairs are would be a readability win.

Could you elaborate what you find more readable about the new style here? I'm genuinely interested.

[therealprof]

I don't want to hunt for the closure argument on the next line, the merging of the pairs (due to fitting in a single line) I don't mind as much.

If anything I'd have preferred:

    uarte.config.write(|w| w
        .hwfc().bit(hardware_flow_control)
        .parity().variant(parity)
    );

or the version rustfmt would have generated for a longer version, like

    uarte.config.write(|w| {
        w.hwfc()
            .bit(hardware_flow_control)
            .parity()
            .variant(parity)
    });

because it highlights exactly the flow of the register manipulation

1. Take the register provided as closure argument
2. Extract the position of the hwfc bits
3. Set the bits to hardware_flow_control
4. Extract the position of the parity bits
5. Set the bits to parity

I'm not too hot about the pair argument since they're not expressed in the API, it's implicit knowledge that you have and easy to get wrong, if anything the API should provide setters instead like:

    uarte.config.write(|w| w.hwfc(hardware_flow_control).parity(parity));

but it doesn't, so the point is moot.

[hannobraun]

If anything I'd have preferred [...] the version rustfmt would have generated for a longer version [...] because it highlights exactly the flow of the register manipulation

I don't see how that version highlights the flow of register manipulation better than any other. The order of operations is the same in all versions. Only whether they are split over lines, and how, changes.

I'm not too hot about the pair argument since they're not expressed in the API, it's implicit knowledge that you have and easy to get wrong

I'm not sure I understand what you mean. The API doesn't use the word "pair", but the method that selects a field and the method that sets its value clearly belong together. The first is a method of w, the second is a method of the type returned by the first method.

If we ignore how the API is structured, they still clearly belong together. One method sets the value of a field and doesn't make any sense without the method that selects which field that is. That you would prefer to have only one method that does both (your last code example) supports that point.

That this isn't clear to everyone is exactly why the formatting should highlight those relationships, not hide them.

if anything the API should provide setters instead like:

I might prefer that too, but as you say, the point is moot.

[jonathanpallant]

Isn't the whole point of machine formatted code to avoid random unrelated style changes? If we don't like what the tool does, we should teach the tool to do what we do like (rustfmt.toml, lints, or whatever) and if it is uncapable of being taught, I think we should fix the tool, or live with it.

One possible workaround is to stop chaining all the calls together:

peripheral.register.write(|w| {
    w.foo().value(1);
    w.bar().value(2);
    unsafe { w.baz().bits(25); }
});

[hannobraun]

Isn't the whole point of machine formatted code to avoid random unrelated style changes?

If that's the case, it's failing spectacularly, since it's doing the exact opposite.

If we don't like what the tool does, we should teach the tool to do what we do like (rustfmt.toml, lints, or whatever) and if it is uncapable of being taught, I think we should fix the tool

Then please go teach the tool how to understand code beyond the syntax level so it can make sensible decisions. Personally, I don't feel like dropping everything I currently do for a decade or so to make that happen.

or live with it.

I don't believe it's self-evident that code should be machine-formatted always in every situation. I know it's supposed to be such a universal solution, but that by itself won't make me roll over against my better judgement.

One possible workaround is to stop chaining all the calls together:

I think that's an excellent suggestion. That still won't make me love rustfmt, but it would take away my biggest gripe.

[hannobraun]

To follow up on my rant on formatting: This is an example where the code didn't get any worse due to the formatting change, but it took me way too long to figure out what had changed about this line, and why that's okay. You're making my life as a reviewer hard for no conceivable benefit.

[therealprof]

That wouldn't be a problem if it was correctly formatted from the start... One of the big benefits of using rustfmt consequently and consistently is that you eliminate random style changes.

[hannobraun]

And I would be all for that, if it didn't come at the cost of less readability in many other places.

[hannobraun]

I don't like this. What if multiple in the dependency graph depend on the HAL, but specify different sizes? Then it won't compile.

This should be added as a generic argument on UarteDMAPoolNode and wherever else it's needed.

[perlindgren]

Agreed its ugly. If I recall it needs to be a constant as we are using the size for allocation. Not sure if we can do it currently in Rust in a more elegant way. I'm open to suggestions here.

[hannobraun]

Ah right. I remember there are limitations around array lengths (although I don't recall the details), so it makes sense you needed to do this to get it to work.

I'm fine with merging it like this for now. Const generics seem to be making progress lately, so hopefully we can fix it by the time someone runs into the dependency graph problems I described.

[perlindgren]

I was aiming at zero-cost and thus avoiding passing arguments and run-time assertions (at least this approach is declarative and zero cost). Regarding Rust's feature handling I'm still hoping for a complete feature algebra, but there might be some soundness things lurking that is blocking. Regarding const generics/eval, there is a lot going on, const assert, etc. Maybe there will be (or even is) a better solution possible.

[therealprof]

You could use typenum to provide the size as a type.

[perlindgren]

Its used to allocate memory, not sure if we have const eval for usize.

[therealprof]

Well, if you use typenum the value is evaluated at compile time so it should be zero cost.

[jonathanpallant]

If typenum is what heapless uses, then we should do that.

[therealprof]

heapless uses generic-array which usestypenum.

[hannobraun]

Should be UartDmaPoolNode as per established convention. I'm all for ignoring conventions when there's a good reason, but in this case, I think the conventional name is easier to parse.

[thalesfragoso]

@korken89 and @jacobrosenthal I copied adapted your idea but with some small changes to get rid of the feature flags, you can see it here:
https://github.com/thalesfragoso/usbd-serial/blob/ba0b51982d16c651b5346a4c24b47cbcc4a9b3f4/src/pool_serial.rs#L1

and a usage example here:
https://gist.github.com/thalesfragoso/e46b68f4d1687713a943ef5d90317b24

It still needs more testing but it looks like it's working fine, let me know what you think. Thanks for this work btw.

Edit: I just realized that there's a problem in PoolPort:: process(), gonna fix it tomorrow.

[korken89]

Thanks for the ping! I am currently a bit swamped, but I'll try and come back on this in the near future.