M-Labs
/
artiq-zynq
9
1
Fork 16
Code Issues 14 Pull Requests 5 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
morgan commented 2023-10-26 13:05:45 +08:00
Owner

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 added 3 commits 2023-10-26 13:05:45 +08:00
77d4ee41e3 replace rustc_cfg[ ] & change write_rustc_cfg_file 
onsolidate all write..file() into config.py
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

Are such refactors suitable for backporting?

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

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

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

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

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
Poster
Owner

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
There is no content yet.
Delete Branch "morgan/artiq-zynq:zc706_sat_warning_fix"

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?