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

Support channel names in RTIO errors #209

Merged
sb10q merged 3 commits from esavkin/artiq-zynq:1697-rtiomap into master 2023-01-09 12:35:56 +08:00
Conversation 4 Commits 3 Files Changed 7 +91 -37
esavkin commented 2022-12-13 12:56:19 +08:00
Owner
Copy Link

Closes https://github.com/m-labs/artiq/issues/1697

Closes https://github.com/m-labs/artiq/issues/1697
esavkin requested review from sb10q 2022-12-13 12:59:48 +08:00
sb10q reviewed 2022-12-13 17:11:50 +08:00
src/runtime/src/rtio_csr.rs Outdated
@ -74,3 +75,2 @@
artiq_raise!("RTIOUnderflow",
"RTIO underflow at {0} mu, channel {1}, slack {2} mu",
timestamp, channel as i64, timestamp - get_counter());
format!("RTIO underflow at channel {}:{}, {{1}} mu, slack {{2}} mu", channel, resolve_channel_name(channel as u32)),
sb10q commented 2022-12-13 17:11:50 +08:00
Owner
Copy Link

No. It's not "at channel".

No. It's not "at channel".
sb10q reviewed 2022-12-13 17:12:41 +08:00
src/runtime/src/rtio_mgt.rs Outdated
@ -335,0 +354,4 @@
}
Ok(())
} ).or_else(|err| {
error!("read_device_map: error reading `device_map` from config: {}", err);
sb10q commented 2022-12-13 17:12:41 +08:00
Owner
Copy Link

Why do you have the "read_device_map:" prefix here and not just above?

Why do you have the "read_device_map:" prefix here and not just above?
sb10q reviewed 2022-12-13 17:15:24 +08:00
src/runtime/src/main.rs Outdated
@ -9,6 +9,7 @@
#![feature(naked_functions)]
#![feature(asm)]
#[macro_use]
sb10q commented 2022-12-13 17:15:24 +08:00
Owner
Copy Link

Why?

Why?
esavkin commented 2022-12-13 17:20:43 +08:00
Author
Owner
Copy Link

format macro wasn't available

format macro wasn't available
sb10q commented 2022-12-13 17:20:14 +08:00
Owner
Copy Link

I guess that works, but it will make porting this firmware to RISC-V later (to merge it with the main ARTIQ runtime) more difficult, since it relies on cache coherency which is not available on MiSoC/RISC-V.

I guess that works, but it will make porting this firmware to RISC-V later (to merge it with the main ARTIQ runtime) more difficult, since it relies on cache coherency which is not available on MiSoC/RISC-V.
esavkin commented 2022-12-13 17:32:45 +08:00
Author
Owner
Copy Link

since it relies on cache coherency

So all resolve_channel_name calls should be made on the core1 (like in RISC-V version now)?

> since it relies on cache coherency So all `resolve_channel_name` calls should be made on the core1 (like in RISC-V version now)?
sb10q commented 2022-12-13 17:38:36 +08:00
Owner
Copy Link

I suppose you meant core0.

As long as we're on Zynq your proposed solution is more or less acceptable (though a bit ugly with the implicit synchronization), but we'll probably have to change it when merging firmwares.

I suppose you meant core0. As long as we're on Zynq your proposed solution is more or less acceptable (though a bit ugly with the implicit synchronization), but we'll probably have to change it when merging firmwares.
esavkin force-pushed 1697-rtiomap from 3bc8c067d3 to 7420a584eb 2022-12-20 17:47:35 +08:00 Compare
esavkin force-pushed 1697-rtiomap from 7420a584eb to b249e69b4c 2022-12-21 10:53:52 +08:00 Compare
esavkin commented 2022-12-21 11:24:19 +08:00
Author
Owner
Copy Link

Changed that behavior so that it is more similar to RISC-V implementation.

Changed that behavior so that it is more similar to RISC-V implementation.
sb10q reviewed 2023-01-03 11:12:39 +08:00
src/runtime/src/comms.rs Outdated
@ -248,2 +248,3 @@
write_i32(stream, exception.id as i32).await?;
write_exception_string(stream, exception.message).await?;
if exception.message.len() == usize::MAX {
sb10q commented 2023-01-03 11:12:26 +08:00
Owner
Copy Link

Obviously, all review comments from the ARTIQ PR are also applicable here.

Obviously, all review comments from the ARTIQ PR are also applicable here.
esavkin marked this conversation as resolved
sb10q reviewed 2023-01-03 11:12:45 +08:00
sb10q reviewed 2023-01-03 11:12:50 +08:00
src/runtime/src/comms.rs Outdated
@ -248,2 +248,3 @@
write_i32(stream, exception.id as i32).await?;
write_exception_string(stream, exception.message).await?;
if exception.message.len() == usize::MAX {
sb10q commented 2023-01-03 11:12:49 +08:00
Owner
Copy Link

Obviously, all review comments from the ARTIQ PR are also applicable here.

Obviously, all review comments from the ARTIQ PR are also applicable here.
sb10q commented 2023-01-03 11:12:51 +08:00
Owner
Copy Link

Obviously, all review comments from the ARTIQ PR are also applicable here.

Obviously, all review comments from the ARTIQ PR are also applicable here.
esavkin marked this conversation as resolved
sb10q reviewed 2023-01-03 11:12:52 +08:00
src/runtime/src/comms.rs Outdated
@ -248,2 +248,3 @@
write_i32(stream, exception.id as i32).await?;
write_exception_string(stream, exception.message).await?;
if exception.message.len() == usize::MAX {
sb10q commented 2023-01-03 11:12:52 +08:00
Owner
Copy Link

Obviously, all review comments from the ARTIQ PR are also applicable here.

Obviously, all review comments from the ARTIQ PR are also applicable here.
esavkin marked this conversation as resolved
sb10q reviewed 2023-01-03 11:12:52 +08:00
src/runtime/src/comms.rs Outdated
@ -248,2 +248,3 @@
write_i32(stream, exception.id as i32).await?;
write_exception_string(stream, exception.message).await?;
if exception.message.len() == usize::MAX {
sb10q commented 2023-01-03 11:12:51 +08:00
Owner
Copy Link

Obviously, all review comments from the ARTIQ PR are also applicable here.

Obviously, all review comments from the ARTIQ PR are also applicable here.
👍 1
sb10q reviewed 2023-01-03 11:15:24 +08:00
src/runtime/src/kernel/dma.rs Outdated
@ -200,3 +200,2 @@
artiq_raise!("RTIOUnderflow",
"RTIO underflow at {0} mu, channel {1}",
timestamp as i64, channel as i64, 0);
"RTIO underflow at {{1}} mu, channel {rtio_channel_info:0}",
sb10q commented 2023-01-03 11:15:24 +08:00
Owner
Copy Link

Why is it {{1}} here and {1} in artiq?

Why is it ``{{1}}`` here and ``{1}`` in ``artiq``?
👍 1
sb10q commented 2023-01-09 12:13:09 +08:00
Owner
Copy Link

This remains unanswered and unaddressed.

This remains unanswered and unaddressed.
esavkin commented 2023-01-09 12:18:48 +08:00
Author
Owner
Copy Link

In this version format! macro is used. In artiq - it is not

In this version `format!` macro is used. In artiq - it is not
sb10q reviewed 2023-01-03 11:17:30 +08:00
src/runtime/src/rtio_mgt.rs Outdated
@ -220,3 +227,3 @@
Ok(Packet::DestinationOkReply) => (),
Ok(Packet::DestinationSequenceErrorReply { channel }) =>{
error!("[DEST#{}] RTIO sequence error involving channel 0x{:04x}", destination, channel);
error!("[DEST#{}] RTIO sequence error involving channel 0x{:04x}:{}", destination, channel, resolve_channel_name(channel));
sb10q commented 2023-01-03 11:17:30 +08:00
Owner
Copy Link

Message format is inconsistent with artiq.

Message format is inconsistent with ``artiq``.
👍 1
esavkin force-pushed 1697-rtiomap from b249e69b4c to 439653b02a 2023-01-04 17:35:47 +08:00 Compare
sb10q merged commit 6b3fa98d70 into master 2023-01-09 12:35:56 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
sb10q
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#209
No description provided.
Delete Branch "esavkin/artiq-zynq:1697-rtiomap"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?