field array support #400

burrbull · 2019-12-14T16:01:15Z

No description provided.

rust-highfive · 2019-12-14T16:01:18Z

r? @Disasm

(rust_highfive has picked a reviewer for you, use r? to override)

burrbull · 2019-12-14T16:01:42Z

bors try

bors · 2019-12-14T16:44:59Z

try

Build succeeded

continuous-integration/travis-ci/push

burrbull · 2019-12-14T19:09:22Z

bors try

bors · 2019-12-14T19:51:24Z

try

Build succeeded

continuous-integration/travis-ci/push

burrbull · 2019-12-15T15:45:53Z

Rebased.

bors try

bors · 2019-12-15T16:26:34Z

try

Build succeeded

continuous-integration/travis-ci/push

Disasm

Thank you! I've found a few things to discuss and/or change.
I checked this change on riscv-rust/k210-pac#33, the output looks good for me despite some pin%s strings in doc comments.

Sorry for the delay.

Disasm · 2019-12-29T08:21:03Z

src/generate/register.rs

@@ -737,6 +856,7 @@ struct F<'a> {
    ty: Ident,
    width: u32,
    write_constraint: Option<&'a WriteConstraint>,
+    dim: Option<(u32, u32, u32, Vec<String>)>,


I believe it will be useful to add a struct to describe these inner tuple fields

src/generate/register.rs

Disasm · 2019-12-29T09:05:03Z

src/generate/register.rs

-                        #_pc_r::new ( #value )
-                    }
-                });
-
                let doc = format!("Reader of field `{}`", f.name);


One of these constructions generates #[doc = "Reader of field `pin%s`"]. Same for the _W type.

Disasm · 2019-12-29T09:22:03Z

src/generate/register.rs

@@ -306,10 +325,10 @@ pub fn fields(
        let _pc_r = &f._pc_r;
        let _pc_w = &f._pc_w;
        let description = &f.description;
-        let description_with_bits = &f.description_with_bits;
+        let width = f.width;


Why do you need this separate variable? It isn't referenced from quote! blocks as far as I can see.

Disasm · 2019-12-29T09:35:17Z

src/generate/register.rs

+                            let sequential_indexes = dim_index
+                                .iter()
+                                .map(|element| element.parse::<u32>())
+                                .eq((first..de.dim+first).map(Ok));


According to the spec dimIndex has 0..1 occurrence and these calculations are redundant.

This check is to ensure that dim_index consist of correct sequence (e.g. 4,5,6,7)

Oh, indeed, I misunderstood the spec.

Disasm · 2019-12-29T09:52:45Z

src/generate/register.rs

+                r_impl_items.push(quote! {
+                    #[doc = #doc]
                    #[inline(always)]
                    pub fn #sc(&self) -> #_pc_r {
                        #_pc_r::new( #value )
                    }
                });


This construction was under if let Some((evs, base)) = lookup_filter(&lookup_results, Usage::Read) {, now it's under } else {. Please check if this is correct, I don't quite understand what's going on here.

src/generate/register.rs

burrbull · 2020-01-01T18:02:11Z

Rebased and fixed.

therealprof · 2020-01-07T10:31:44Z

src/generate/register.rs

@@ -216,26 +213,19 @@ pub fn fields(
    let can_write = access != Access::ReadOnly;

    // TODO enumeratedValues
+    let inline = quote! { #[inline(always)] };


Have you checked whether this is worth the harder to read quote sections below? If we do something like this we ideally wouldn't repeat the quoting for each register we process but do it exactly once and then reuse it.

Disasm · 2020-01-07T10:40:21Z

I'd rather prefer readability over speed, because we already have relatively fast processing, but the code is quite difficult to understand.

therealprof · 2020-01-07T10:47:19Z

I'd rather prefer readability over speed, because we already have relatively fast processing, but the code is quite difficult to understand.

I agree. Also we could do a lot better explaining what the code is doing, especially for the more obscure and less common parts.

425: replace_suffix r=therealprof a=burrbull Part of #400 Co-authored-by: Andrey Zgarbul <[email protected]>

src/generate/register.rs

Disasm

Overall LGTM. I think we can merge this after discussion on the last two comments (with or without corresponding fixes).

Disasm · 2020-01-08T18:01:18Z

src/generate/register.rs

@@ -246,7 +239,6 @@ pub fn fields(
        let fty = width.to_ty()?;
        let evs = &f.enumerated_values;
        let quotedfield = String::from("`") + &f.name + "`";


Maybe this let quotedfield line should be moved too. @therealprof could you comment on this?

No preference. I planned to do something with it but that plan is already gone out the window with this PR anyway. 😅

Ok! @burrbull, could you move it then to line 296?

Disasm · 2020-01-08T18:02:04Z

src/generate/register.rs

-                        let pc = base.field.to_sanitized_upper_case();
+                        let pc = util::replace_suffix(base.field, "")
+                            .to_sanitized_upper_case()
+                            .to_string();


Do you need this .to_string();?

CH_ABORT_R

This field does not contain <dim>
https://github.com/riscv-rust/k210-pac/blob/4f7330528951b6d1f54ed5030d7f17b0078cdce5/k210.svd#L1123
Please add there:

<dim>6</dim> <dimIncrement>1</dimIncrement> <dimIndex>1-6</dimIndex>

Disasm · 2020-01-08T18:16:41Z

Hmm, there are still buggy doc comments:

        #[doc = "Reader of field `ch%s_abort`"]
        pub type CH_ABORT_R = crate::R<bool, bool>;
        #[doc = "Write proxy for field `ch%s_abort`"]
        pub struct CH_ABORT_W<'a> {
            w: &'a mut W,
        }

Disasm · 2020-01-08T18:21:18Z

Also (ch_abort_we(1-6) instead of ch(1-6)_abort_we)

        #[doc = "Reader of fields `ch_abort_we(1-6)`"]
        pub type CH_ABORT_WE_R = crate::R<bool, bool>;
        #[doc = "Write proxy for fields `ch_abort_we(1-6)`"]
        pub struct CH_ABORT_WE_W<'a> {
            w: &'a mut W,
            offset: usize,
        }

But sometimes it generates correct docs:

            #[doc = "Enable write to ch(1-6)_susp bit"]
            #[inline(always)]
            pub unsafe fn ch_susp_we(&self, n: usize) -> CH_SUSP_WE_R {
                CH_SUSP_WE_R::new(((self.bits >> n - 1 + 24) & 0x01) != 0)
            }

src/generate/register.rs

burrbull · 2020-01-08T20:20:52Z

I've already shut down my PC. But you can do it yourself. :)

Disasm · 2020-01-08T20:26:32Z

Indeed!

Disasm

Thank you @burrbull!

Disasm · 2020-01-08T20:28:39Z

bors r+

400: field array support r=Disasm a=burrbull Co-authored-by: Andrey Zgarbul <[email protected]> Co-authored-by: Zgarbul Andrey <[email protected]> Co-authored-by: Vadim Kaushan <[email protected]>

bors · 2020-01-08T21:04:31Z

Build failed

continuous-integration/travis-ci/push

burrbull · 2020-01-09T05:39:39Z

cargo fmt and squash commits

Disasm

bors r+

400: field array support r=Disasm a=burrbull Co-authored-by: Andrey Zgarbul <[email protected]>

bors · 2020-01-09T07:22:35Z

Build succeeded

continuous-integration/travis-ci/push

rust-highfive assigned Disasm Dec 14, 2019

rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Dec 14, 2019

bors bot added a commit that referenced this pull request Dec 14, 2019

Try #400:

48a446a

burrbull force-pushed the field-array branch 2 times, most recently from 5b77117 to bedc69f Compare December 14, 2019 19:08

bors bot added a commit that referenced this pull request Dec 14, 2019

Try #400:

4d523d6

burrbull changed the title ~~[WIP] field array support~~ field array support Dec 14, 2019

burrbull marked this pull request as ready for review December 14, 2019 19:50

burrbull requested a review from a team as a code owner December 14, 2019 19:50

This was referenced Dec 14, 2019

update + field arrays riscv-rust/k210-pac#33

Closed

field arrays stm32-rs/stm32-rs#320

Closed

burrbull force-pushed the field-array branch from ce572bd to ff56f52 Compare December 15, 2019 15:45

bors bot added a commit that referenced this pull request Dec 15, 2019

Try #400:

30d13df

Sh3Rm4n mentioned this pull request Dec 22, 2019

Use gpio variants stm32-rs/stm32f3xx-hal#42

Merged

Disasm suggested changes Dec 29, 2019

View reviewed changes

Disasm added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2019

burrbull force-pushed the field-array branch 3 times, most recently from 1ca7988 to 5a2cbea Compare January 1, 2020 18:00

burrbull force-pushed the field-array branch from 5a2cbea to 852da47 Compare January 1, 2020 18:06

burrbull force-pushed the field-array branch from 42dc466 to 5db3b82 Compare January 7, 2020 10:21

therealprof reviewed Jan 7, 2020

View reviewed changes

burrbull mentioned this pull request Jan 7, 2020

replace_suffix #425

Merged

bors bot added a commit that referenced this pull request Jan 7, 2020

Merge #425

7d3cdf6

425: replace_suffix r=therealprof a=burrbull Part of #400 Co-authored-by: Andrey Zgarbul <[email protected]>

burrbull force-pushed the field-array branch from 5db3b82 to 768894b Compare January 7, 2020 12:24

Disasm reviewed Jan 7, 2020

View reviewed changes

src/generate/register.rs Outdated Show resolved Hide resolved

src/generate/register.rs Outdated Show resolved Hide resolved

Disasm approved these changes Jan 8, 2020

View reviewed changes

Disasm reviewed Jan 8, 2020

View reviewed changes

src/generate/register.rs Outdated Show resolved Hide resolved

Disasm reviewed Jan 8, 2020

View reviewed changes

src/generate/register.rs Outdated Show resolved Hide resolved

Disasm approved these changes Jan 8, 2020

View reviewed changes

bors bot added a commit that referenced this pull request Jan 8, 2020

Merge #400

91a7bdb

400: field array support r=Disasm a=burrbull Co-authored-by: Andrey Zgarbul <[email protected]> Co-authored-by: Zgarbul Andrey <[email protected]> Co-authored-by: Vadim Kaushan <[email protected]>

field array support & review fixes

abbe720

burrbull force-pushed the field-array branch from 882bf0b to abbe720 Compare January 9, 2020 05:38

Disasm approved these changes Jan 9, 2020

View reviewed changes

bors bot added a commit that referenced this pull request Jan 9, 2020

Merge #400

d2fb516

400: field array support r=Disasm a=burrbull Co-authored-by: Andrey Zgarbul <[email protected]>

bors bot merged commit abbe720 into master Jan 9, 2020

bors bot deleted the field-array branch January 9, 2020 07:22

Disasm mentioned this pull request Dec 2, 2020

K210.svd is malformed riscv-rust/k210-pac#35

Closed

adamgreig mentioned this pull request Mar 28, 2021

use u8 for field array offset instead of usize #495

Closed

Dirbaio mentioned this pull request Dec 13, 2021

Support field arrays #44

Closed

field array support #400

field array support #400

Conversation

burrbull commented Dec 14, 2019

rust-highfive commented Dec 14, 2019

burrbull commented Dec 14, 2019

bors bot commented Dec 14, 2019

try

Build succeeded

burrbull commented Dec 14, 2019

bors bot commented Dec 14, 2019

try

Build succeeded

burrbull commented Dec 15, 2019

bors bot commented Dec 15, 2019

try

Build succeeded

Disasm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Disasm Dec 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burrbull commented Jan 1, 2020

Choose a reason for hiding this comment

Disasm commented Jan 7, 2020

therealprof commented Jan 7, 2020

Disasm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burrbull Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Disasm commented Jan 8, 2020

Disasm commented Jan 8, 2020 • edited Loading

burrbull commented Jan 8, 2020

Disasm commented Jan 8, 2020

Disasm left a comment

Choose a reason for hiding this comment

Disasm commented Jan 8, 2020

bors bot commented Jan 8, 2020

Build failed

burrbull commented Jan 9, 2020

Disasm left a comment

Choose a reason for hiding this comment

bors bot commented Jan 9, 2020

Build succeeded

Disasm Dec 29, 2019 •

edited

Loading

burrbull Jan 8, 2020 •

edited

Loading

Disasm commented Jan 8, 2020 •

edited

Loading