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
Owner

update my last PR (#242) to use csr::virtual_leds and general touch up

update my last PR (#242) to use csr::virtual_leds and general touch up
mwojcik reviewed 2023-08-01 16:32:15 +08:00
@ -136,2 +126,2 @@
SEEN_RTIO_LED = READ_RTIO_LED;
};
let _ = block_async!(wait_for_virtual_leds_change()).await;
info!("switching");
Owner

Debug leftover? Remove pls

Debug leftover? Remove pls
morgan marked this conversation as resolved
mwojcik reviewed 2023-08-01 16:35:29 +08:00
@ -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();
Owner

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

Could save a CSR call by getting the value before the ``if``
morgan marked this conversation as resolved
sb10q reviewed 2023-08-02 17:44:59 +08:00
@ -673,2 +656,4 @@
let mut hardware_tick_ts = 0;
let mut i2c0 = I2c::i2c0();
let mut i2c1 = I2c::i2c0();
Owner

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.
sb10q reviewed 2023-08-02 17:46:41 +08:00
sb10q reviewed 2023-08-02 17:48:13 +08:00
@ -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();
Owner

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.
morgan force-pushed feature from aafa07df80 to 85da57980a 2023-08-04 14:30:54 +08:00 Compare
sb10q reviewed 2023-08-04 15:41:03 +08:00
@ -105,0 +107,4 @@
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
fn wait_for_virtual_leds_change() -> nb::Result<(), Void> {
unsafe {
Owner

The only unsafe line is the next one, no?

The only unsafe line is the next one, no?
Author
Owner

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
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:41:40 +08:00
@ -105,0 +118,4 @@
}
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
async fn async_io_expanders_service() {
Owner

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

``async_`` in the name sounds redundant since this is declared ``async fn
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:42:28 +08:00
@ -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;
Owner

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() }; ... ```
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:43:51 +08:00
@ -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();
Owner

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?
sb10q reviewed 2023-08-04 15:45:26 +08:00
@ -11,2 +13,4 @@
}
//IO expanders pins
const IO_DIR_INPUT_ALL: u8 = 0xFF;
Owner

Doesn't seem like this constant adds anything to the code.

Doesn't seem like this constant adds anything to the code.
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:46:02 +08:00
@ -13,0 +22,4 @@
const IO_DIR_OUT_SFP0_LED: u8 = !0x80;
//IO expander port direction
const IO_DIR_MAPPING0: [u8; 2] = [
Owner

It's just called "iodir", not "mapping"

It's just called "iodir", not "mapping"
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:46:27 +08:00
@ -12,1 +14,4 @@
//IO expanders pins
const IO_DIR_INPUT_ALL: u8 = 0xFF;
const IO_DIR_OUT_SFP_TX_DISABLE: u8 = !0x02;
Owner

Move the inversions into the iodir definition.

Move the inversions into the iodir definition.
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:47:50 +08:00
@ -102,0 +139,4 @@
0 => IO_DIR_MAPPING0,
1 => IO_DIR_MAPPING1,
_ => [IO_DIR_INPUT_ALL; 2],
};
Owner

Aren't you doing that already at object creation above?

Aren't you doing that already at object creation above?
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:48:01 +08:00
@ -18,2 +40,4 @@
out_target: [u8; 2],
registers: Registers,
virtual_led_mapping: &'static [(u8, u8, u8)],
channel: usize,
Owner

What is the purpose of that channel field?

What is the purpose of that channel field?
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:50:33 +08:00
@ -150,5 +174,8 @@ pub fn main_core0() {
task::spawn(report_async_rtio_errors());
Owner

As you can see this is called report_async_rtio_errors, not async_report_async_rtio_errors.

Asynchronous RTIO errors have nothing to do with async/await.

As you can see this is called ``report_async_rtio_errors``, not ``async_report_async_rtio_errors``. Asynchronous RTIO errors have nothing to do with async/await.
morgan marked this conversation as resolved
sb10q reviewed 2023-08-04 15:53:53 +08:00
@ -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();
Owner

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.
morgan force-pushed feature from cf8eaabd78 to aa073514b9 2023-08-10 13:22:19 +08:00 Compare
sb10q reviewed 2023-08-13 14:48:09 +08:00
@ -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))]
Owner

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

I don't think this should be gated on has_drtio.
sb10q reviewed 2023-08-13 14:50:38 +08:00
@ -105,0 +114,4 @@
io_expander1: RefCell<io_expander::IoExpander>,
) {
loop {
let _ = block_async!((|| -> nb::Result<(), Void> {
Owner

I don't think this entire block_async is any useful. It also duplicates logic and breaks abstraction layers. Anyway just remove it.

I don't think this entire block_async is any useful. It also duplicates logic and breaks abstraction layers. Anyway just remove it.
Author
Owner

The block_async prevent "io_expanders_service" from locking up the whole cpu. As it blocks the loop from running over and over again.

The block_async prevent "io_expanders_service" from locking up the whole cpu. As it blocks the loop from running over and over again.
morgan force-pushed feature from aa073514b9 to 5279358640 2023-08-18 13:35:01 +08:00 Compare
sb10q reviewed 2023-08-28 16:03:15 +08:00
@ -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)]
Owner

needs update

needs update
morgan force-pushed feature from 5279358640 to cfe53a57a3 2023-08-28 16:06:31 +08:00 Compare
morgan added 1 commit 2023-08-28 16:07:05 +08:00
sb10q merged commit 622d267d55 into master 2023-08-28 16:08:11 +08:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/artiq-zynq#244
No description provided.