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

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
morgan added 1 commit 2023-07-31 10:54:39 +08:00
morgan added 1 commit 2023-08-01 16:21:25 +08:00
morgan added 1 commit 2023-08-01 16:29:46 +08:00
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");

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();

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
morgan added 1 commit 2023-08-01 16:59:12 +08:00
3d66c57247 - remove debug message
- save 1 csr call
morgan added 1 commit 2023-08-02 17:42:10 +08:00
aafa07df80 - inline i2c0..1 into io_expander
- remove redundant i2c.init
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();

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();

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
morgan added 1 commit 2023-08-04 14:32:28 +08:00
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 {

The only unsafe line is the next one, no?

The only unsafe line is the next one, no?
Poster
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() {

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;

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();

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;

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] = [

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;

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],
};

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,

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());

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();

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))]

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> {

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.
Poster
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)]

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
There is no content yet.