Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unconditional is None # Can only have one unconditional assignment per field #93

Open
xachb opened this issue Apr 3, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@xachb
Copy link

xachb commented Apr 3, 2024

Hi Alex,

I managed to hit an assert and looking at the SystemRDL 2.0 Spec it is not clear to me why that is happening. I found empirically that I can hit the assert when I try to instantiate a field from the following field definition:

field srw_1s__hr {
    name = "srw_1s__hr";
    desc = "Default description";
    sw = rw;
    singlepulse = true;
    hw = rw;
};

There may be something nonsensical about what I am doing there, but nevertheless I hit it accidently and the assert was not helpful for figuring out what in my .rdl code was causing it. I intended to have hw = r; and that will cleared the assert;

I am using peakrdl-regblock==0.22.0 and the assert is on line 222 of field_logic/generators.py, and reads:

assert unconditional is None # Can only have one unconditional assignment per field

Perhaps an update to the assert message would help, or maybe there is actually a bug there? I super appreciate this project and appreciate all that you are doing.

Thank you,
Zach

@amykyta3
Copy link
Member

amykyta3 commented Apr 4, 2024

Hey Zach!
Thanks for submitting this. I agree that the tool should have provided a better error message here. I usually add assertions to trap conditions that shouldn't ever happen regardless of user input. It looks like you were able to stir up a rare edge-case I didn't expect.

Analyzing this a bit more, the specific conflict that is happening is that an unqualified hw=w policy along with singlepulse describes a field that doesn't quite make sense:

  • hw=w (or rw in your case) without a load-enable from we = true implies that hardware will update the value of the field's storage element every clock cycle unconditionally.
  • singlepulse = true dictates the field will be cleared back to 0 at the next clock cycle, also unconditionally.

This combination of properties is impossible to satisfy, so it results in an error. The SystemRDL spec doesn't explicitly specify all the possible conflicts you could create, likely because there would be far too many to enumerate in a document 😄 .

I'll definitely fix the error message so that it is more informative.
Thanks again for submitting this!

@amykyta3 amykyta3 added the bug Something isn't working label Apr 4, 2024
@mkahane
Copy link
Contributor

mkahane commented Jun 21, 2024

Hey all! I also ran into this on a very similar register... What I am trying to do (and I assume what OP wanted) is a normal status register that exposes a continuous hardware signal readable by software that is overloaded to pulse a 1 on a separate output when it is written. The solution I found is to do something like this:

reg reg_i {
    field  {
        desc = "status field";
        sw = r;
        hw = w;
    } status[0:0];

    field  {
        desc = "pulse field";
        sw = w;
        hw = r;
        singlepulse = 1;
        reset = 0;
    } pulse[0:0];
};

Alex you may suggest that it would be better to make these different fields that occupy different bit positions, but in my case I need to make this a drop in replacement for an existing software driver that assumes this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants