Repository navigation
Refactoring of registers via wrappers #27
Description
Context
If you look at serial.rs, this is the current situation:
At the top, we have some const defined
//FIFO enabled.
const IIR_FIFO_BITS: u8 = 0b1100_0000;
const IIR_RDA_BIT: u8 = 0b0000_0100;
const IIR_NONE_BIT: u8 = 0b0000_0001;
const IIR_THR_EMPTY_BIT: u8 = 0b0000_0010;
The pattern is <register>_<field_name>.
An example of usage in the wild is:
fn add_interrupt(&mut self, interrupt_bits: u8) {
self.interrupt_identification &= !IIR_NONE_BIT;
self.interrupt_identification |= interrupt_bits;
}
Problem
- The naming scheme suggests a possible grouping of these consts.
- It's not intuitive what's going on with that code, due to manual bit manipulation.
Proposal
First proposal
I know it's not preferable to include external dependencies like bitfield but I think I simple macro will do the trick.
I would propose a macro which will allow the following:
/// Interrupt Identification Register
defreg!(IIR, u8, [
FIFO[8-7],
RDA[2-2],
THR_EMPTY[1-1],
NONE[0-0],
]);
// Example of usage:
fn add_interrupt(&mut self, interrupt_bits: u8) {
self.interrupt_identification.clear_bit(IIR::NONE);
self.interrupt_identification.set_bit(interrupt_bits);
}
The parameters are: the struct name, size of the wrapped value, and list of fields and position of those fields (e.g. NONE is one bit long, and it's the lsb).
We could also add another input for a default value (and remove all the DEFAULTconsts).
Second proposal
A simple struct based wrapper. Playground.
#[derive(Debug)]
struct IIR(u8);
impl IIR{
const FIFO_BITS: u8 = 0b1100_0000;
const NONE_BIT: u8 = 0b0000_0001;
const THR_EMPTY_BIT: u8 = 0b0000_0010;
const RDA_BIT: u8 = 0b0000_0100;
}
impl default::Default for IIR{
fn default() -> Self{
Self(IIR::NONE_BIT)
}
}
We can then define the similar methods as above (clear_bit and set_bit).
Activity
andreeaflorescu commentedon Feb 4, 2021
Generally grouping the registers together sounds very nice. I think we can go further down and provide abstractions for the main components of the serial console. For example, if we take the interrupts, there are 3 main operations (which have separate defines):
-trigering the interrupt
A non significant overhead in the serial console comes from handling interrupts. If we can "modularize" working with an interrupt, we get "for free" the separation you are proposing because we can then define the bit shifts in the context of the newly created interface. Does that make sense?
FedericoPonzi commentedon Feb 8, 2021
I'm not sure I understand correctly your proposal, can you maybe show an example of the interface you're referrng to? I thnk you're proposing to not expose the bit masks, but instead to provide methods like "set_bit_x".