Generic Part1 by burrbull · Pull Request #347 · rust-embedded/svd2rust

burrbull · 2019-07-29T07:21:17Z

Rebased and reopened.

rust-highfive · 2019-07-29T07:21:20Z

r? @therealprof

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

burrbull · 2019-07-31T07:20:10Z

cc @therealprof

~~With this PR debug builds grow 1.5% (I think it is because Deref), but source now much cleaner.~~

Now all right.

It also depends on #358.

burrbull · 2019-07-31T09:51:08Z

cc @Disasm

burrbull · 2019-07-31T10:59:50Z

@therealprof I've rebased this on master. Test, please.

Disasm · 2019-07-31T16:16:56Z

src/generate/generic.rs

+        impl<U, REG> Reg<U, REG>
+        where
+            Self: Writable,
+            U: Copy + Default


Is this Default equals to "returns zero"?

Yes, it is for bool, u8, u16 u32 and u64.

Yes, but these types are not enforced I guess

They are enforced by size type of register or field.

But it's possible to create a wrapper with the same size and a custom Default impl that returns reset value, for example

How? All fields are private. You can get register only from peripheral as it was before.

Hmm, now I see. Thanks.

Disasm · 2019-07-31T16:19:27Z

Send and !Sync were implemented on registers before, with this change they are not implemented.
UPD: I'm wrong, they are implemented for Reg, but with bounds on the generic parameters.

burrbull · 2019-07-31T16:32:52Z

Send and !Sync were implemented on registers before, with this change they are not implemented.

It was auto-implementation?

Disasm · 2019-07-31T16:35:29Z

Yes, I think that was due to the inner VolatileCell: it's also Send and !Sync.

Disasm · 2019-07-31T16:38:25Z

As discussed on Matrix, documentation becomes even more difficult to understand with this change: it's unclear how to read and write registers, also there is no information about field variants.

therealprof · 2019-07-31T16:38:26Z

TBH I'm not too sure whether we really need that VolatileCell anyway once we've remodelled everything. 😅

Disasm · 2019-07-31T16:39:43Z

@therealprof Yes, but it's better to have Send and !Sync anyway, because they add important properties.

burrbull · 2019-07-31T16:42:24Z

error[E0658]: negative trait bounds are not yet fully implemented; use marker types for now

What to do with this?

Disasm · 2019-07-31T16:44:44Z

I think that PhantomData with some weird type can help

burrbull · 2019-07-31T16:46:29Z

Can you write how you see implementation for !Sync?

Disasm · 2019-07-31T16:54:23Z

Seems like !Sync is already implemented for Reg, but as for Send it's difficult to say whether it's implemented or not for a specific register.

burrbull · 2019-07-31T17:35:59Z

I've added Send and slightly improved docs.

burrbull · 2019-07-31T18:40:13Z

What you mean there is no information about field variants?

Disasm · 2019-07-31T18:42:14Z

@burrbull It's no longer accessible from the register documentation pages. Only on upper levels.

burrbull · 2019-07-31T18:48:56Z

@burrbull It's no longer accessible from the register documentation pages. Only on upper levels.

Can you show example?

Disasm · 2019-07-31T18:59:52Z

spi0::RegisterBlock -> CTRLR0 -> R -> WORK_MODER

burrbull · 2019-07-31T19:47:49Z

Here is one breaking change (#354). Reader now have variant method:

same as writer:

And now you can use this enum for both read and write:

let v = spi.ctrl0.read().work_mode().variant();
...
spi.ctrl0.write(|w| w.work_mode().variant(v)); // Before now incompatible types error was here.

burrbull · 2019-08-01T05:15:50Z

@therealprof I can't find where is source of this documentation https://docs.rs/svd2rust ?

therealprof · 2019-08-01T07:14:21Z

@burrbull You mean src/lib.rs?

Disasm · 2019-08-01T07:55:16Z

src/generate/generic.rs

+            ///Register size
+            type Size;
+            ///Reset value of the register
+            fn reset_value() -> Self::Size;


This associated type name is debatable, but I'm not going to force my point of view here.

I've changed Size on Type

therealprof · 2019-08-01T08:26:50Z

src/generate/register.rs


+    let doc = format!("`read()` method returns [{0}::R]({0}::R) reader structure", &name_sc);
+    if can_read {
+        out.push(quote! {


Why not move the let doc = ... inside the if clause? Should be cheaper to compute and avoids the risk of accidentally using it in a wrong context.

therealprof · 2019-08-01T08:27:05Z

src/generate/register.rs

+        });
+    }
+    let doc = format!("`write(|w| ..)` method takes [{0}::W]({0}::W) writer structure", &name_sc);
+    if can_write {


therealprof · 2019-08-01T08:29:00Z

Also please rebase and make sure the commits have a meaningful commit message.

burrbull · 2019-08-01T08:32:08Z

Rebased.

therealprof · 2019-08-01T08:34:21Z

bors try

bors · 2019-08-01T09:20:54Z

try

Build succeeded

continuous-integration/travis-ci/push

therealprof · 2019-08-02T08:59:35Z

@Disasm Still happy?

Disasm · 2019-08-02T09:23:20Z

@therealprof yes, looks good.

Disasm · 2019-08-02T09:26:45Z

In general, I'd improve documentation for generic methods (read/write/...), replacing references to the book with short examples.

Disasm · 2019-08-02T09:33:13Z

bors r+

347: Generic Part1 r=Disasm a=burrbull Rebased and reopened. Co-authored-by: Andrey Zgarbul <zgarbul.andrey@gmail.com>

bors · 2019-08-02T10:13:56Z

Build succeeded

continuous-integration/travis-ci/push

rust-highfive assigned therealprof Jul 29, 2019

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

burrbull force-pushed the generics-p1 branch from b06af77 to 7d5477a Compare July 31, 2019 06:05

burrbull marked this pull request as ready for review July 31, 2019 07:14

burrbull requested a review from a team as a code owner July 31, 2019 07:14

burrbull force-pushed the generics-p1 branch 2 times, most recently from ef62cc3 to 9fd21ec Compare July 31, 2019 09:50

burrbull force-pushed the generics-p1 branch from 9fd21ec to 5cc5972 Compare July 31, 2019 10:57

Disasm reviewed Jul 31, 2019

View reviewed changes

burrbull force-pushed the generics-p1 branch from 2c63db4 to 383ab91 Compare July 31, 2019 18:13

burrbull force-pushed the generics-p1 branch from 7e9056a to 7b9d119 Compare August 1, 2019 06:21

Disasm previously approved these changes Aug 1, 2019

View reviewed changes

burrbull dismissed Disasm’s stale review via 55a1651 August 1, 2019 08:21

therealprof reviewed Aug 1, 2019

View reviewed changes

replace registers with generic Reg type

e7b3539

burrbull force-pushed the generics-p1 branch from 44dde42 to e7b3539 Compare August 1, 2019 08:31

bors bot added a commit that referenced this pull request Aug 1, 2019

Try #347:

f84b507

Disasm approved these changes Aug 2, 2019

View reviewed changes

bors bot added a commit that referenced this pull request Aug 2, 2019

Merge #347

d0c547f

347: Generic Part1 r=Disasm a=burrbull Rebased and reopened. Co-authored-by: Andrey Zgarbul <zgarbul.andrey@gmail.com>

bors bot merged commit e7b3539 into rust-embedded:master Aug 2, 2019

burrbull deleted the generics-p1 branch August 3, 2019 15:57

Conversation

burrbull commented Jul 29, 2019

Uh oh!

rust-highfive commented Jul 29, 2019

Uh oh!

burrbull commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Disasm commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

Disasm commented Jul 31, 2019

Uh oh!

Disasm commented Jul 31, 2019

Uh oh!

therealprof commented Jul 31, 2019

Uh oh!

Disasm commented Jul 31, 2019

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

Disasm commented Jul 31, 2019

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

Disasm commented Jul 31, 2019

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

Disasm commented Jul 31, 2019

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

Disasm commented Jul 31, 2019

Uh oh!

burrbull commented Jul 31, 2019

Uh oh!

burrbull commented Aug 1, 2019

Uh oh!

therealprof commented Aug 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

therealprof commented Aug 1, 2019

Uh oh!

burrbull commented Aug 1, 2019

Uh oh!

therealprof commented Aug 1, 2019

Uh oh!

bors bot commented Aug 1, 2019

try

Build succeeded

burrbull commented Jul 31, 2019 •

edited

Loading

Disasm commented Jul 31, 2019 •

edited

Loading