Skip to content
This repository has been archived by the owner on Jul 6, 2019. It is now read-only.

ioreg tweaks #141

Closed
wants to merge 2 commits into from
Closed

ioreg tweaks #141

wants to merge 2 commits into from

Conversation

bharrisau
Copy link
Contributor

Two minor changes to ioreg for registers with only write-only fields. I'll throw some tests in later.

closes #139
closes #140

@bgamari
Copy link
Contributor

bgamari commented Aug 27, 2014

Wait, is bors still with us?

@dpc
Copy link
Contributor

dpc commented Aug 28, 2014

Fails for me.

_k20.o  /home/dpc/lab/rust/zinc/apps/app_blink_k20.rs 
<quote expansion>:5:38: 7:65 error: type `&'static reg::WDOG_unlock` does not implement any method in scope named `set`
<quote expansion>:5     name_1,ctxt_0.name_86,ctxt_0.name_231,ctxt_0((name_237,ctxt_0 as
<quote expansion>:6                                                             name_214,ctxt_0)
<quote expansion>:7                                                            << 0);
error: aborting due to previous error
rake aborted!

@bgamari
Copy link
Contributor

bgamari commented Aug 28, 2014

@dpc hmm, let me have a look. Which branch are you working from?

@bharrisau
Copy link
Contributor Author

@bgamari You did the r+ backwards?
On 28/08/2014 8:15 am, "Ben Gamari" notifications@github.com wrote:

@dpc https://github.com/dpc hmm, let me have a look.


Reply to this email directly or view it on GitHub
#141 (comment).

@bharrisau
Copy link
Contributor Author

@dpc You want to use set_unlock, not just set.
On 28/08/2014 8:12 am, "Dawid Ciężarkiewicz" notifications@github.com
wrote:

Fails for me.

_k20.o /home/dpc/lab/rust/zinc/apps/app_blink_k20.rs
:5:38: 7:65 error: type &'static reg::WDOG_unlock does not implement any method in scope named set
:5 name_1,ctxt_0.name_86,ctxt_0.name_231,ctxt_0((name_237,ctxt_0 as
:6 name_214,ctxt_0)
:7 << 0);
error: aborting due to previous error
rake aborted!


Reply to this email directly or view it on GitHub
#141 (comment).

@bgamari
Copy link
Contributor

bgamari commented Aug 28, 2014

@bharrisau ahem, yes I did. I'll try that again.

@dpc
Copy link
Contributor

dpc commented Aug 28, 2014

Also, I might not fully understand this immediate patch, but I think it's not best approach. Writes to wo register can be buffered and chained etc. It's a nice feature and easy to understand. The only thing that should be different, is during creation, their initial value should be some default (like 0), instead of read from the underlying register.

@bgamari
Copy link
Contributor

bgamari commented Aug 28, 2014

@dpc that is a fair point. @bharrisau is there a reason you chose to eliminate buffering in this case?

@dpc
Copy link
Contributor

dpc commented Aug 28, 2014

@bharrisau: i do use set_unlock

      0xe => reg16 unlock {                                                                                                              
        0..15 => unlock: wo                                                                                                              /,                                                                                                                         
      },  

and latter:

    reg::WDOG.unlock.set_unlock(0xc520);                                                                                                 
    reg::WDOG.unlock.set_unlock(0xd928);

It works without bb46d6e

@bharrisau
Copy link
Contributor Author

Yeah, hold off on the r+. I did it up last night because you wanted to
chain two immediate writes. It is more semantically correct to do it as you
suggest and just remove the read.

Once you post your watchdog module as a PR we will hand a better chance of
diagnosing the other issue.
On 28/08/2014 8:27 am, "Dawid Ciężarkiewicz" notifications@github.com
wrote:

@bharrisau https://github.com/bharrisau: i do use set_unlock

  0xe => reg16 unlock {
    0..15 => unlock: wo                                                                                                              /,
  },

and latter:

reg::WDOG.unlock.set_unlock(0xc520);
reg::WDOG.unlock.set_unlock(0xd928);

It works without bb46d6e
bb46d6e


Reply to this email directly or view it on GitHub
#141 (comment).

@bgamari
Copy link
Contributor

bgamari commented Aug 28, 2014

@dpc @bharrisau how does this look? bgamari@0d8d9c6

@dpc
Copy link
Contributor

dpc commented Aug 28, 2014

I need to do two consecutive writes of two different values to the same register. But it is seems a read between them is breaking the sequence and hardware will not register such sequence as valid.

I guess chaining could be confusing for users in case someone wanted to do:

reg::WDOG.unlock.set_unlock(0xc520).set_unlock(0xd928);

and hopping it is going to perform two writes, but I think this is not a big deal, and it is easy to understand that chaining is used precisely for merging accesses.

@bgamari
Copy link
Contributor

bgamari commented Aug 28, 2014

@dpc: correct.

As you point out, it's important that users understand when it's alright to use chaining. In particular, chained operations will be combined into a single read-modify-write cycle. This means only the last update to a given field in a chain will be committed. Ideally we'd have a lint to ensure that users don't accidentally step into this trap.

For this reason I think it's important that we remain consistent in the semantics of chaining and not perform immediate writes in the case of write-only registers.

@bharrisau
Copy link
Contributor Author

In the future a lint or the type system will be able to check for mistakes
like that.

@dpc
Copy link
Contributor

dpc commented Aug 28, 2014

@bgamari: bgamari/zinc@0d8d9c6 Looks OK and produces good assembler, so I think it's OK. I'll try with the hardware in few hours.

@bharrisau bharrisau closed this Aug 28, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ioreg should do an immediate write when all fields are wo ioreg macro breaks with "wo" field.
3 participants