M-Labs
/
artiq-zynq
9
1
Fork 16
Code Issues 14 Pull Requests 5 Releases Wiki Activity

Add SFP fibers support for satman #217

Merged
sb10q merged 2 commits from esavkin/artiq-zynq:213-satman-sfp-fiber-support into master 2023-02-17 17:19:30 +08:00
Conversation 3 Commits 2 Files Changed 4
esavkin commented 2023-02-17 14:50:50 +08:00
Owner

Make io_expander and i2c wrapper code shared by moving it from runtime to libboard_artiq.

Closes #213

Needs to be merged to release-7 branch.

Make io_expander and i2c wrapper code shared by moving it from runtime to libboard_artiq. Closes #213 Needs to be merged to release-7 branch.
sb10q commented 2023-02-17 14:55:43 +08:00
Owner

Why was the busno parameter removed? Again please keep this PR and the refactoring to one thing. The changes like i2c.write -> i2c::write, while perhaps not incorrect unlike the busno change, do not belong here.

I suspect you have not tested this properly on runtime. https://github.com/m-labs/artiq/blob/master/artiq/coredevice/i2c.py

Why was the busno parameter removed? Again please keep this PR and the refactoring to one thing. The changes like i2c.write -> i2c::write, while perhaps not incorrect unlike the busno change, do not belong here. I suspect you have not tested this properly on runtime. https://github.com/m-labs/artiq/blob/master/artiq/coredevice/i2c.py
esavkin commented 2023-02-17 15:01:23 +08:00
Poster
Owner

I suspect you have not tested this properly on runtime. https://github.com/m-labs/artiq/blob/master/artiq/coredevice/i2c.py

I see, I didn't expect external API for this. I'll move the i2c wrapper back and untie the io_expander from wrapper

> I suspect you have not tested this properly on runtime. https://github.com/m-labs/artiq/blob/master/artiq/coredevice/i2c.py I see, I didn't expect external API for this. I'll move the i2c wrapper back and untie the io_expander from wrapper
sb10q commented 2023-02-17 15:02:42 +08:00
Owner

I see, I didn't expect external API for this.

The fact that it raises exceptions should have made alarm bells go off. Normally there are no exceptions in Rust.

> I see, I didn't expect external API for this. The fact that it raises exceptions should have made alarm bells go off. Normally there are no exceptions in Rust.
sb10q reviewed 2023-02-17 16:34:03 +08:00
src/runtime/src/main.rs Outdated
@ -134,2 +119,2 @@
io_expander0.service().unwrap();
io_expander1.service().unwrap();
let i2c = unsafe { (&mut i2c::I2C_BUS).as_mut().unwrap() };
for i_i2c in 0..2 {
sb10q commented 2023-02-17 16:34:03 +08:00
Owner

expander_index, expander_i or just i

``expander_index``, ``expander_i`` or just ``i``
👍 1
esavkin force-pushed 213-satman-sfp-fiber-support from fe1be9e272 to 394e2b2354 2023-02-17 16:54:52 +08:00 Compare
sb10q reviewed 2023-02-17 16:56:04 +08:00
src/satman/src/main.rs Outdated
@ -452,0 +452,4 @@
#[cfg(feature = "target_kasli_soc")]
{
for expander_i in &[0u8, 1] {
let mut io_expander = io_expander::IoExpander::new(&mut i2c, *expander_i).unwrap();
sb10q commented 2023-02-17 16:56:04 +08:00
Owner

I doubt the & and * are necessary here.

I doubt the & and * are necessary here.
esavkin commented 2023-02-17 16:57:19 +08:00
Poster
Owner

& makes a slice from array (to make it iterable)

& makes a slice from array (to make it iterable)
sb10q commented 2023-02-17 16:58:50 +08:00
Owner

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6f1f8688caff98619cc0d08dd6d99a27

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6f1f8688caff98619cc0d08dd6d99a27
esavkin commented 2023-02-17 17:02:22 +08:00
Poster
Owner

https://github.com/rust-lang/rust/issues/84837

error[E0277]: `[u8; 2]` is not an iterator
firmware>    --> runtime/src/main.rs:119:27
firmware>     |
firmware> 119 |         for expander_i in [0u8, 1] {
firmware>     |                           ^^^^^^^^ borrow the array with `&` or call `.iter()` on it to iterate over it
firmware>     |
firmware>     = help: the trait `Iterator` is not implemented for `[u8; 2]`
firmware>     = note: arrays are not iterators, but slices like the following are: `&[1, 2, 3]`
firmware>     = note: required because of the requirements on the impl of `IntoIterator` for `[u8; 2]`
firmware>     = note: required by `into_iter`
firmware> error: aborting due to previous error
firmware> For more information about this error, try `rustc --explain E0277`.
https://github.com/rust-lang/rust/issues/84837 ```rust error[E0277]: `[u8; 2]` is not an iterator firmware> --> runtime/src/main.rs:119:27 firmware> | firmware> 119 | for expander_i in [0u8, 1] { firmware> | ^^^^^^^^ borrow the array with `&` or call `.iter()` on it to iterate over it firmware> | firmware> = help: the trait `Iterator` is not implemented for `[u8; 2]` firmware> = note: arrays are not iterators, but slices like the following are: `&[1, 2, 3]` firmware> = note: required because of the requirements on the impl of `IntoIterator` for `[u8; 2]` firmware> = note: required by `into_iter` firmware> error: aborting due to previous error firmware> For more information about this error, try `rustc --explain E0277`. ```
sb10q commented 2023-02-17 17:05:43 +08:00
Owner

Doesn't seem relevant to this case?

Doesn't seem relevant to this case?
esavkin commented 2023-02-17 17:08:08 +08:00
Poster
Owner

Maybe the link is not really relevant, but the compiler output is

Maybe the link is not really relevant, but the compiler output is
sb10q commented 2023-02-17 17:11:02 +08:00
Owner

Seems to be a peculiar issue with the particular compiler version we use here. What was the problem with 0..2 that you had written earlier?

Seems to be a peculiar issue with the particular compiler version we use here. What was the problem with 0..2 that you had written earlier?
esavkin commented 2023-02-17 17:12:23 +08:00
Poster
Owner

0..2 may seem be confusing, because only 0 and 1 are used, from my point of view

0..2 may seem be confusing, because only 0 and 1 are used, from my point of view
sb10q commented 2023-02-17 17:12:49 +08:00
Owner

You can expect developers to know range boundaries.

You can expect developers to know range boundaries.
👀 1
sb10q commented 2023-02-17 17:21:31 +08:00
Owner

There's also the 0..=1 syntax...

There's also the ``0..=1`` syntax...
sb10q reviewed 2023-02-17 16:56:49 +08:00
src/runtime/src/main.rs Outdated
@ -142,3 +136,3 @@
}
};
sb10q commented 2023-02-17 16:56:49 +08:00
Owner

...

...
esavkin commented 2023-02-17 17:06:24 +08:00
Poster
Owner

Maybe leave it as it is now? IDE does such things all the time

Maybe leave it as it is now? IDE does such things all the time
sb10q commented 2023-02-17 17:06:45 +08:00
Owner

Then reconfigure or change your IDE.

Then reconfigure or change your IDE.
sb10q commented 2023-02-17 17:08:01 +08:00
Owner

Or run a whitespace cleanup on the entire codebase in a separate PR.

Or run a whitespace cleanup on the entire codebase in a separate PR.
esavkin commented 2023-02-17 17:11:25 +08:00
Poster
Owner

Or run a whitespace cleanup on the entire codebase in a separate PR.

Maybe integrate some standard tool for formatting files with appropriate config in the future?

> Or run a whitespace cleanup on the entire codebase in a separate PR. Maybe integrate some standard tool for formatting files with appropriate config in the future?
sb10q commented 2023-02-17 17:12:21 +08:00
Owner

Sure, if you find something that's easy to integrate, easy to enforce (e.g. git commit hook?), works well, and isn't proprietary.

Sure, if you find something that's easy to integrate, easy to enforce (e.g. git commit hook?), works well, and isn't proprietary.
esavkin commented 2023-02-17 17:13:02 +08:00
Poster
Owner

For example https://github.com/rust-lang/rustfmt

For example https://github.com/rust-lang/rustfmt
esavkin force-pushed 213-satman-sfp-fiber-support from 394e2b2354 to 8822f2493e 2023-02-17 17:18:17 +08:00 Compare
sb10q merged commit 05c22792d6 into master 2023-02-17 17:19:30 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
priority:critical
   
priority:high
   
priority:low
   
priority:medium
No Label
priority:critical
priority:high
priority:low
priority:medium
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 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#217
There is no content yet.
Delete Branch "esavkin/artiq-zynq:213-satman-sfp-fiber-support"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?