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

release-7: fix zc706 satellite compilation warning #275

Merged
sb10q merged 1 commits from morgan/artiq-zynq:zc706_sat_warning_fix into release-7 2023-11-07 13:47:25 +08:00
Conversation 2 Commits 1 Files Changed 1 +1
morgan commented 2023-10-26 13:05:45 +08:00
Owner
Copy Link

Summary

  • add has_rtio_moninj cfg to fix unused variables warning related to drtioaux::Packet

  • the root cause of the compilation warning is due to absent of has_rtio_moninj in zc706 satellite's cfg. Which gated the codes that use those parameters.

    • For example for the MonitorRequest function, the warning show channel and probe being unused due to the csr::rtio_moninj::mon_chan_sel_write(channel as _); and csr::rtio_moninj::mon_probe_sel_write(probe); being gated by #[cfg(has_rtio_moninj)].
// at satman main.rs
...
drtioaux::Packet::MonitorRequest { destination: _destination, channel, probe } => {
    forward!(_routing_table, _destination, *_rank, _repeaters, &packet, timer);
    let value;
    #[cfg(has_rtio_moninj)]   // <----- gated without patch & channel and probe are not used
    unsafe {
        csr::rtio_moninj::mon_chan_sel_write(channel as _);
        csr::rtio_moninj::mon_probe_sel_write(probe);
        csr::rtio_moninj::mon_value_update_write(1);
        value = csr::rtio_moninj::mon_value_read() as u64;
    }
    #[cfg(not(has_rtio_moninj))]
    {
        value = 0;
    }
    let reply = drtioaux::Packet::MonitorReply { value: value };
    drtioaux::send(0, &reply)
},
...
  • compilation check
    • run nix build <variant> -L of the following and all built successfully
    • zc706-acpki_nist_qc2-jtag
    • zc706-acpki_nist_qc2_master-jtag
    • zc706-acpki_nist_qc2_satellite-jtag

Compilation Warning before patch

nix build .#zc706-nist_qc2_satellite-jtag -L 
...
firmware>    --> satman/src/main.rs:211:71
firmware>     |
firmware> 211 |         drtioaux::Packet::MonitorRequest { destination: _destination, channel, probe } => {
firmware>     |                                                                       ^^^^^^^ help: try ignoring the field: `channel: _`
firmware>     |
firmware>     = note: `#[warn(unused_variables)]` on by default
firmware> warning: unused variable: `probe`
firmware>    --> satman/src/main.rs:211:80
firmware>     |
firmware> 211 |         drtioaux::Packet::MonitorRequest { destination: _destination, channel, probe } => {
firmware>     |                                                                                ^^^^^ help: try ignoring the field: `probe: _`
firmware> warning: unused variable: `channel`
firmware>    --> satman/src/main.rs:228:73
firmware>     |
firmware> 228 |         drtioaux::Packet::InjectionRequest { destination: _destination, channel, 
firmware>     |                                                                         ^^^^^^^ help: try ignoring the field: `channel: _`
firmware> warning: unused variable: `overrd`
firmware>    --> satman/src/main.rs:229:46
firmware>     |
firmware> 229 | ...                   overrd, value } => {
firmware>     |                       ^^^^^^ help: try ignoring the field: `overrd: _`
firmware> warning: unused variable: `value`
firmware>    --> satman/src/main.rs:229:54
firmware>     |
firmware> 229 | ...                   overrd, value } => {
firmware>     |                               ^^^^^ help: try ignoring the field: `value: _`
firmware> warning: unused variable: `channel`
firmware>    --> satman/src/main.rs:240:52
firmware>     |
firmware> 240 | ...                   channel, overrd } => {
firmware>     |                       ^^^^^^^ help: try ignoring the field: `channel: _`
firmware> warning: unused variable: `overrd`
firmware>    --> satman/src/main.rs:240:61
firmware>     |
firmware> 240 | ...                   channel, overrd } => {
firmware>     |                                ^^^^^^ help: try ignoring the field: `overrd: _`
firmware> warning: 7 warnings emitted
## Summary - add `has_rtio_moninj` cfg to fix unused variables warning related to `drtioaux::Packet` - the root cause of the compilation warning is due to absent of `has_rtio_moninj` in zc706 satellite's cfg. Which gated the codes that use those parameters. - For example for the `MonitorRequest` function, the warning show `channel` and `probe` being unused due to the `csr::rtio_moninj::mon_chan_sel_write(channel as _);` and `csr::rtio_moninj::mon_probe_sel_write(probe);` being gated by `#[cfg(has_rtio_moninj)]`. ```rust // at satman main.rs ... drtioaux::Packet::MonitorRequest { destination: _destination, channel, probe } => { forward!(_routing_table, _destination, *_rank, _repeaters, &packet, timer); let value; #[cfg(has_rtio_moninj)] // <----- gated without patch & channel and probe are not used unsafe { csr::rtio_moninj::mon_chan_sel_write(channel as _); csr::rtio_moninj::mon_probe_sel_write(probe); csr::rtio_moninj::mon_value_update_write(1); value = csr::rtio_moninj::mon_value_read() as u64; } #[cfg(not(has_rtio_moninj))] { value = 0; } let reply = drtioaux::Packet::MonitorReply { value: value }; drtioaux::send(0, &reply) }, ... ``` - compilation check - run `nix build <variant> -L` of the following and all built successfully - `zc706-acpki_nist_qc2-jtag` - `zc706-acpki_nist_qc2_master-jtag` - `zc706-acpki_nist_qc2_satellite-jtag` ## Compilation Warning before patch ```bash nix build .#zc706-nist_qc2_satellite-jtag -L ... firmware> --> satman/src/main.rs:211:71 firmware> | firmware> 211 | drtioaux::Packet::MonitorRequest { destination: _destination, channel, probe } => { firmware> | ^^^^^^^ help: try ignoring the field: `channel: _` firmware> | firmware> = note: `#[warn(unused_variables)]` on by default firmware> warning: unused variable: `probe` firmware> --> satman/src/main.rs:211:80 firmware> | firmware> 211 | drtioaux::Packet::MonitorRequest { destination: _destination, channel, probe } => { firmware> | ^^^^^ help: try ignoring the field: `probe: _` firmware> warning: unused variable: `channel` firmware> --> satman/src/main.rs:228:73 firmware> | firmware> 228 | drtioaux::Packet::InjectionRequest { destination: _destination, channel, firmware> | ^^^^^^^ help: try ignoring the field: `channel: _` firmware> warning: unused variable: `overrd` firmware> --> satman/src/main.rs:229:46 firmware> | firmware> 229 | ... overrd, value } => { firmware> | ^^^^^^ help: try ignoring the field: `overrd: _` firmware> warning: unused variable: `value` firmware> --> satman/src/main.rs:229:54 firmware> | firmware> 229 | ... overrd, value } => { firmware> | ^^^^^ help: try ignoring the field: `value: _` firmware> warning: unused variable: `channel` firmware> --> satman/src/main.rs:240:52 firmware> | firmware> 240 | ... channel, overrd } => { firmware> | ^^^^^^^ help: try ignoring the field: `channel: _` firmware> warning: unused variable: `overrd` firmware> --> satman/src/main.rs:240:61 firmware> | firmware> 240 | ... channel, overrd } => { firmware> | ^^^^^^ help: try ignoring the field: `overrd: _` firmware> warning: 7 warnings emitted ```
morgan force-pushed zc706_sat_warning_fix from c4ea3a7bf3 to 7d9b007a16 2023-10-26 13:10:08 +08:00 Compare
morgan changed title from release-7: consoldiate write...file(), update zc706.py and fix zc706 satellite compilation warning to release-7: consoldiate write...file() and fix zc706 satellite compilation warning 2023-10-26 13:18:38 +08:00
morgan force-pushed zc706_sat_warning_fix from 7d9b007a16 to e3d01fb7ac 2023-10-26 13:21:40 +08:00 Compare
esavkin reviewed 2023-10-27 09:52:47 +08:00
src/gateware/config.py Outdated
@ -0,0 +1,16 @@
from misoc.integration import cpu_interface
esavkin commented 2023-10-27 09:52:47 +08:00
Owner
Copy Link

Are such refactors suitable for backporting?

Are such refactors suitable for backporting?
mwojcik commented 2023-10-27 10:17:20 +08:00
Owner
Copy Link

Better than copying code from misoc, IMO.

No harm in also introducing config.py like on the curernt master. Compatibility isn't broken, and that's what's most important for backporting.

Better than copying code from ``misoc``, IMO. No harm in also introducing config.py like on the curernt master. Compatibility isn't broken, and that's what's most important for backporting.
sb10q commented 2023-10-27 15:19:35 +08:00
Owner
Copy Link

No, generally we should be conservative on release branches, and not make any changes that are not necessary to fix bugs.

No, generally we should be conservative on release branches, and not make any changes that are not necessary to fix bugs.
sb10q commented 2023-10-27 15:20:08 +08:00
Owner
Copy Link

Introducing this refactoring would be ok if a bug fix depends on it, but as far as I can tell this is not the case here.

Introducing this refactoring would be ok if a bug fix depends on it, but as far as I can tell this is not the case here.
👍 1
morgan force-pushed zc706_sat_warning_fix from e3d01fb7ac to 131c7103df 2023-10-27 09:58:53 +08:00 Compare
morgan changed title from release-7: consoldiate write...file() and fix zc706 satellite compilation warning to release-7: update zc706.py to follow kasli_soc.py and fix zc706 satellite compilation warning 2023-10-27 10:02:10 +08:00
sb10q commented 2023-11-01 17:44:30 +08:00
Owner
Copy Link

The patch still looks wrong.

The patch still looks wrong.
morgan force-pushed zc706_sat_warning_fix from 131c7103df to fab45fb6f9 2023-11-02 12:36:30 +08:00 Compare
morgan commented 2023-11-02 12:40:02 +08:00
Author
Owner
Copy Link

Force push to simplify the patch.

Force push to simplify the patch.
morgan changed title from release-7: update zc706.py to follow kasli_soc.py and fix zc706 satellite compilation warning to release-7: fix zc706 satellite compilation warning 2023-11-02 12:40:19 +08:00
sb10q merged commit 6573ecd487 into release-7 2023-11-07 13:47:24 +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
4 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#275
No description provided.
Delete Branch "morgan/artiq-zynq:zc706_sat_warning_fix"

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?