use csr::virtual_leds for SFP0..3 LED indication #244
No reviewers
Labels
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: M-Labs/artiq-zynq#244
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "morgan/artiq-zynq:feature"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
update my last PR (#242) to use csr::virtual_leds and general touch up
@ -136,2 +126,2 @@
SEEN_RTIO_LED = READ_RTIO_LED;
};
let _ = block_async!(wait_for_virtual_leds_change()).await;
info!("switching");
Debug leftover? Remove pls
@ -118,2 +111,2 @@
}
if READ_RTIO_LED != SEEN_RTIO_LED {
if pl::csr::virtual_leds::status_read() != LAST_VIRTUAL_LED_STATUS {
LAST_VIRTUAL_LED_STATUS = pl::csr::virtual_leds::status_read();
Could save a CSR call by getting the value before the
if
@ -673,2 +656,4 @@
let mut hardware_tick_ts = 0;
let mut i2c0 = I2c::i2c0();
let mut i2c1 = I2c::i2c0();
No. This object should be created only one time for the entire firmware.
@ -126,2 +120,4 @@
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
async fn async_rtio_led() {
let mut io_expander0 = io_expander::IoExpander::new(unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }, 0).unwrap();
let mut io_expander1 = io_expander::IoExpander::new(unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }, 1).unwrap();
It's confusing to create IO expander objects, which also should be singletons and do more than controlling LEDs, into a function whose name indicates it is about LEDs.
aafa07df80
to85da57980a
@ -105,0 +107,4 @@
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
fn wait_for_virtual_leds_change() -> nb::Result<(), Void> {
unsafe {
The only unsafe line is the next one, no?
LAST_VIRTUAL_LED_STATUS
is a static mut. So it need an unsafe too@ -105,0 +118,4 @@
}
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
async fn async_io_expanders_service() {
async_
in the name sounds redundant since this is declared ``async fn@ -105,0 +122,4 @@
let mut io_expander0 = io_expander::IoExpander::new(unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }, 0).unwrap();
let mut io_expander1 = io_expander::IoExpander::new(unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }, 1).unwrap();
loop {
let _ = block_async!(wait_for_virtual_leds_change()).await;
Just inline it.
@ -658,2 +657,4 @@
let mut hardware_tick_ts = 0;
let mut io_expander0 = io_expander::IoExpander::new(unsafe { &mut *i2c_ptr }, 0).unwrap();
let mut io_expander1 = io_expander::IoExpander::new(unsafe { &mut *i2c_ptr }, 1).unwrap();
Better than before but is there really no other solution than disabling the borrow checker?
@ -11,2 +13,4 @@
}
//IO expanders pins
const IO_DIR_INPUT_ALL: u8 = 0xFF;
Doesn't seem like this constant adds anything to the code.
@ -13,0 +22,4 @@
const IO_DIR_OUT_SFP0_LED: u8 = !0x80;
//IO expander port direction
const IO_DIR_MAPPING0: [u8; 2] = [
It's just called "iodir", not "mapping"
@ -12,1 +14,4 @@
//IO expanders pins
const IO_DIR_INPUT_ALL: u8 = 0xFF;
const IO_DIR_OUT_SFP_TX_DISABLE: u8 = !0x02;
Move the inversions into the iodir definition.
@ -102,0 +139,4 @@
0 => IO_DIR_MAPPING0,
1 => IO_DIR_MAPPING1,
_ => [IO_DIR_INPUT_ALL; 2],
};
Aren't you doing that already at object creation above?
@ -18,2 +40,4 @@
out_target: [u8; 2],
registers: Registers,
virtual_led_mapping: &'static [(u8, u8, u8)],
channel: usize,
What is the purpose of that channel field?
@ -150,5 +174,8 @@ pub fn main_core0() {
task::spawn(report_async_rtio_errors());
As you can see this is called
report_async_rtio_errors
, notasync_report_async_rtio_errors
.Asynchronous RTIO errors have nothing to do with async/await.
@ -105,0 +120,4 @@
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
async fn async_io_expanders_service() {
let mut io_expander0 = io_expander::IoExpander::new(unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }, 0).unwrap();
let mut io_expander1 = io_expander::IoExpander::new(unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() }, 1).unwrap();
As I told you before, those I/O expander objects should be created only once in the entire firmware.
Additionally, you create them once with
i2c::I2C_BUS
and once with&mut *i2c_ptr
, which is inconsistent.cf8eaabd78
toaa073514b9
@ -105,0 +107,4 @@
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
static mut LAST_VIRTUAL_LED_STATUS: u8 = 0;
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
I don't think this should be gated on has_drtio.
@ -105,0 +114,4 @@
io_expander1: RefCell<io_expander::IoExpander>,
) {
loop {
let _ = block_async!((|| -> nb::Result<(), Void> {
I don't think this entire block_async is any useful. It also duplicates logic and breaks abstraction layers. Anyway just remove it.
The block_async prevent "io_expanders_service" from locking up the whole cpu. As it blocks the loop from running over and over again.
aa073514b9
to5279358640
@ -470,0 +486,4 @@
self.csr_devices.append("virtual_leds")
self.comb += [self.virtual_leds.get(i).eq(channel.rx_ready)
for i, channel in enumerate(self.drtio_transceiver.channels)]
needs update
5279358640
tocfe53a57a3