M-Labs/artiq-zynq
10
1
Fork 20
Code Issues 13 Pull Requests 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 +29 -12
3 changed files with 29 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 28aa03b291 - Show all commits

24
src/libboard_artiq/src/io_expander.rs
View File

@ -39,16 +39,27 @@ pub struct IoExpander<'a> {
out_current: [u8; 2],
out_target: [u8; 2],
registers: Registers,
virtual_led_mapping: &'static [(u8, u8, u8)],
channel: usize,
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:48:01 +08:00
Outdated
Review
Copy Link

What is the purpose of that channel field?

What is the purpose of that channel field?
}
impl<'a> IoExpander<'a> {
pub fn new(i2c: &'a mut i2c::I2c, index: u8) -> Result<Self, &'static str> {
#[cfg(hw_rev = "v1.0")]
const VIRTUAL_LED_MAPPING0: [(u8, u8, u8); 2] = [(0, 0, 6), (1, 1, 6)];
#[cfg(hw_rev = "v1.1")]
const VIRTUAL_LED_MAPPING0: [(u8, u8, u8); 2] = [(0, 0, 7), (1, 1, 6)];
const VIRTUAL_LED_MAPPING1: [(u8, u8, u8); 2] = [(2, 0, 6), (3, 1, 6)];
// Both expanders on SHARED I2C bus
let mut io_expander = match index {
0 => IoExpander {
i2c,
channel: 0,
address: 0x40,
iodir: [0xff; 2],
virtual_led_mapping: &VIRTUAL_LED_MAPPING0,
iodir: IO_DIR_MAPPING0,
out_current: [0; 2],
out_target: [0; 2],
registers: Registers {
@ -60,8 +71,10 @@ impl<'a> IoExpander<'a> {
},
1 => IoExpander {
i2c,
channel: 1,
address: 0x42,
iodir: [0xff; 2],
virtual_led_mapping: &VIRTUAL_LED_MAPPING1,
iodir: IO_DIR_MAPPING1,
out_current: [0; 2],
out_target: [0; 2],
registers: Registers {
@ -121,6 +134,13 @@ impl<'a> IoExpander<'a> {
pub fn init(&mut self) -> Result<(), &'static str> {
self.select()?;
self.iodir = match self.channel {
0 => IO_DIR_MAPPING0,
1 => IO_DIR_MAPPING1,
_ => [IO_DIR_INPUT_ALL; 2],
};
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:47:50 +08:00
Outdated
Review
Copy Link

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

Aren't you doing that already at object creation above?
self.update_iodir()?;
self.out_current[0] = 0x00;

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

@ -118,7 +118,7 @@ fn wait_for_virtual_leds_change() -> nb::Result<(), Void> {
}
}
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
async fn async_rtio_led() {
async fn async_io_expanders_service() {
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 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();
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.
loop {
@ -156,8 +156,6 @@ pub fn main_core0() {
let mut io_expander = io_expander::IoExpander::new(i2c, expander_i).unwrap();
io_expander.init().expect("I2C I/O expander #0 initialization failed");
// Actively drive TX_DISABLE to false on SFP0..3
io_expander.set_oe(0, 1 << 1).unwrap();
io_expander.set_oe(1, 1 << 1).unwrap();
io_expander.set(0, 1, false);
io_expander.set(1, 1, false);
io_expander.service().unwrap();
@ -176,5 +174,8 @@ pub fn main_core0() {
task::spawn(report_async_rtio_errors());
morgan marked this conversation as resolved Outdated
sb10q commented 2023-08-04 15:50:33 +08:00
Outdated
Review
Copy Link

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.
#[cfg(all(feature = "target_kasli_soc", has_drtio))]
task::spawn(async_io_expanders_service());
comms::main(timer, cfg);
}

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

@ -610,6 +610,7 @@ pub extern "C" fn main_core0() -> i32 {
ram::init_alloc_core0();
let mut i2c = I2c::i2c0();
let i2c_ptr = &mut i2c as *mut I2c;
i2c.init().expect("I2C initialization failed");
#[cfg(feature = "target_kasli_soc")]
{
@ -617,8 +618,6 @@ pub extern "C" fn main_core0() -> i32 {
let mut io_expander = io_expander::IoExpander::new(&mut i2c, expander_i).unwrap();
io_expander.init().expect("I2C I/O expander #0 initialization failed");
// Actively drive TX_DISABLE to false on SFP0..3
io_expander.set_oe(0, 1 << 1).unwrap();
io_expander.set_oe(1, 1 << 1).unwrap();
io_expander.set(0, 1, false);
io_expander.set(1, 1, false);
io_expander.service().unwrap();
@ -657,11 +656,8 @@ pub extern "C" fn main_core0() -> i32 {
let mut hardware_tick_ts = 0;
let mut i2c0 = I2c::i2c0();
let mut i2c1 = I2c::i2c0();
i2c0.init().expect("I2C0 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();
let mut io_expander0 = io_expander::IoExpander::new(unsafe { &mut *i2c_ptr }, 0).unwrap();
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 io_expander1 = io_expander::IoExpander::new(unsafe { &mut *i2c_ptr }, 1).unwrap();
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?
loop {
while !drtiosat_link_rx_up() {