-
Notifications
You must be signed in to change notification settings - Fork 79
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
[RFC] Support both Layers as const-array and as slices #96
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK in the idea.
type CustomAction; | ||
|
||
/// Retrieve the action at given `layer` for key at `(row, col)`. | ||
fn get_action(&self, layer: usize, row: u8, col: u8) -> Option<&Action<Self::CustomAction>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe some coord type would improve code readability and remove col/row missmatches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean something like struct Coords { row: u8, col: u8 }
(as (u8, u8)
and struct Coords(u8, u8)
don't solve the problem)? Or maybe some wrapper types similar to stm32f0xx_hal::time::Hertz, like struct Row(u8)
, struct Col(u8)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean struct Coord { i: u8, j: u8 }
but that's a refactor that might be out of scope of this PR.
@@ -15,6 +15,10 @@ | |||
#![no_std] | |||
#![deny(missing_docs)] | |||
|
|||
#[cfg(test)] | |||
#[macro_use] | |||
extern crate alloc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think I don't. I must have put it there when testing something (maybe using vec!
somewhere) and it should be safe to remove.
Another issue could be the indirection. key-matrix polling could be considered a perf-sensitive hot-loop (latency, for example). Not too familiar with how it works in the embedded world, or if the indirection ultimately gets optimised away, but, having the layout known at compile time would allow the compiler to make some nice optimisations, no? |
Here, the types will be monomorphised at compile time, so it is a "zero cost abstraction", meaning that you'll get a code as fast as if you had implemented by hand on the two case. So it should not be an issue. |
FYI: I like this solution 👍 |
This is a quick proof-of-concept of a potential solution to #88.
Instead of having Layers as concrete type, it is a trait and is auto-implemented for both
&'a [&'a [&'a [Action<T>]]]
and[[[Action<T>; C]; R]; L]
. This way it is possible to limit the number of type parameters passed. So one could use something like:or if the
CustomAction
associated type has to limited (because e.g. we are adding handling of our actions):The syntax for defining layers stays quite simple, either
static LAYERS: LayersArray<2, 1, 2> = [...]
orstatic LAYERS: LayersSlice = &[...]
(but it's probably better to use the first one). We could even have some different aliases, like e.g.which would be more convenient for users.
From my initial tests the binary size does not increase, so it seems like everything gets optimized out properly, but I didn't do any strict testing, just running
cargo size --release
.This was just a quick idea, it's open for discussion and would require code cleanup and renaming a few things.
@riskable Please check if this approach would actually help to simplify your interface.