M-Labs/artiq-zynq
10
1
Fork 20
Code Issues 13 Pull Requests 1 Releases Wiki Activity

use csr::virtual_leds for SFP0..3 LED indication #244

Merged
sb10q merged 10 commits from morgan/artiq-zynq:feature into master 2023-08-28 16:08:11 +08:00
Conversation 0 Commits 10 Files Changed 4 +27 -25
2 changed files with 27 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit cee22f43aa - Show all commits

33
src/runtime/src/main.rs
View File

@ -103,19 +103,13 @@ async fn report_async_rtio_errors() {
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
static mut SEEN_RTIO_LED: u8 = 0;
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
static mut READ_RTIO_LED: u8 = 0;
static mut LAST_VIRTUAL_LED_STATUS: u8 = 0;
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
fn wait_for_rtio_led_change() -> nb::Result<(), Void> {
fn wait_for_virtual_leds_change() -> nb::Result<(), Void> {
unsafe {
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:41:03 +08:00
Outdated
Review
Copy Link

The only unsafe line is the next one, no?

The only unsafe line is the next one, no?
morgan commented 2023-08-04 15:48:54 +08:00
Outdated
Review
Copy Link

LAST_VIRTUAL_LED_STATUS is a static mut. So it need an unsafe too

```LAST_VIRTUAL_LED_STATUS``` is a static mut. So it need an unsafe too
sb10q commented 2023-08-13 14:48:09 +08:00
Outdated
Review
Copy Link

I don't think this should be gated on has_drtio.

I don't think this should be gated on has_drtio.
let len: usize = pl::csr::DRTIO.len();
READ_RTIO_LED = 0;
for linkno in 0..len {
READ_RTIO_LED |= (pl::csr::DRTIO[linkno].rx_up_read)() << linkno;
}
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();
morgan marked this conversation as resolved Outdated
mwojcik commented 2023-08-01 16:35:29 +08:00
Outdated
Review
Copy Link

Could save a CSR call by getting the value before the if

Could save a CSR call by getting the value before the ``if``
Ok(())
} else {
Err(nb::Error::WouldBlock)
@ -124,15 +118,16 @@ fn wait_for_rtio_led_change() -> nb::Result<(), Void> {
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
async fn async_rtio_led() {
let _ = block_async!(wait_for_rtio_led_change()).await;
unsafe {
let i2c = (&mut i2c::I2C_BUS).as_mut().unwrap();
for expander_i in 0..=1 {
let mut io_expander = io_expander::IoExpander::new(i2c, expander_i).unwrap();
io_expander.service().expect("I2C I/O expander service failed");
}
SEEN_RTIO_LED = READ_RTIO_LED;
};
let i2c0 = unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() };
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:41:40 +08:00
Outdated
Review
Copy Link

async_ in the name sounds redundant since this is declared ``async fn

``async_`` in the name sounds redundant since this is declared ``async fn
let i2c1 = unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() };
let mut io_expander0 = io_expander::IoExpander::new(i2c0, 0).unwrap();
sb10q commented 2023-08-02 17:48:13 +08:00
Outdated
Review
Copy Link

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.

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.
sb10q commented 2023-08-04 15:53:53 +08:00
Outdated
Review
Copy Link

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.

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.
let mut io_expander1 = io_expander::IoExpander::new(i2c1, 1).unwrap();
loop{
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:42:28 +08:00
Outdated
Review
Copy Link

Just inline it.

 block_async!(|| {
   let current = unsafe { pl::csr::virtual_leds::status_read() };
   ...
Just inline it. ``` block_async!(|| { let current = unsafe { pl::csr::virtual_leds::status_read() }; ... ```
let _ = block_async!(wait_for_virtual_leds_change()).await;
info!("switching");
morgan marked this conversation as resolved Outdated
mwojcik commented 2023-08-01 16:32:15 +08:00
Outdated
Review
Copy Link

Debug leftover? Remove pls

Debug leftover? Remove pls
io_expander0.service().expect("I2C I/O expander #0 service failed");
io_expander1.service().expect("I2C I/O expander #1 service failed");
}
}
static mut LOG_BUFFER: [u8; 1 << 17] = [0; 1 << 17];

19
src/satman/src/main.rs
View File

@ -657,6 +657,13 @@ pub extern "C" fn main_core0() -> i32 {
let mut hardware_tick_ts = 0;
sb10q commented 2023-08-02 17:44:59 +08:00
Outdated
Review
Copy Link

No. This object should be created only one time for the entire firmware.

No. This object should be created only one time for the entire firmware.
let mut i2c0 = I2c::i2c0();
sb10q commented 2023-08-04 15:43:51 +08:00
Outdated
Review
Copy Link

Better than before but is there really no other solution than disabling the borrow checker?

Better than before but is there really no other solution than disabling the borrow checker?
let mut i2c1 = I2c::i2c0();
i2c0.init().expect("I2C0 initialization failed");
i2c1.init().expect("I2C1 initialization failed");
let mut io_expander0 = io_expander::IoExpander::new(&mut i2c0, 0).unwrap();
let mut io_expander1 = io_expander::IoExpander::new(&mut i2c1, 1).unwrap();
loop {
while !drtiosat_link_rx_up() {
drtiosat_process_errors();
@ -665,9 +672,9 @@ pub extern "C" fn main_core0() -> i32 {
rep.service(&routing_table, rank, &mut timer);
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
for expander_i in 0..=1 {
let mut io_expander = io_expander::IoExpander::new(&mut i2c, expander_i).unwrap();
io_expander.service().expect("I2C I/O expander service failed")
{
io_expander0.service().expect("I2C I/O expander #0 service failed");
io_expander1.service().expect("I2C I/O expander #1 service failed");
}
hardware_tick(&mut hardware_tick_ts, &mut timer);
@ -707,9 +714,9 @@ pub extern "C" fn main_core0() -> i32 {
rep.service(&routing_table, rank, &mut timer);
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
for expander_i in 0..=1 {
let mut io_expander = io_expander::IoExpander::new(&mut i2c, expander_i).unwrap();
io_expander.service().expect("I2C I/O expander service failed")
{
io_expander0.service().expect("I2C I/O expander #0 service failed");
io_expander1.service().expect("I2C I/O expander #1 service failed");
}
hardware_tick(&mut hardware_tick_ts, &mut timer);
if drtiosat_tsc_loaded() {