-
Notifications
You must be signed in to change notification settings - Fork 61
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
document coverUnreachable and strict options for the switch block #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just suggesting some rewording here. Please choose what you prefer between my suggestion, and yours. otherwise LGTM
Co-authored-by: wifasoi <[email protected]>
I like that very much, I was aware it was a bit difficult. But I thought any attempt is better than none and someone can help me with that :) Glad for the suggestion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have never used SpinalHDL enumerated types yet (except indirectly via FSM) so I am discovering things here and I review this PR as a newbie
^^^^^^^^^^^^^^^^^^ | ||
|
||
Sometimes handling all cases can become unwieldy and error prone so SpinalHDL, by default, will handle the uncovered cases with the last ``is`` block. | ||
To explicitly declare and define a ``default`` block the option ``coverUnreachable`` can be passed to the switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to understand correctly what it is about here so I suggest another way to explain.
If I understand correctly there are two kinds of "cover" to do (I use my words here but feel free to use other words if they are clearer):
- "logic" cover: the possible values that can be applied on the wires from the logic (enumerated types)
- "bits" cover: all possible combination of 0s and 1s in the wires. It is a superset of "logic" cover.
For example, using this encoding in an enumerated type (maybe provide an example with actual code)
MOV -> b00
ADD -> b10
SUB -> b11
"logic" cover requires covering the 3 cases MOV, ADD and SUB and "bits" cover requires covering the 4 cases b00 to b11, which are the 3 cases from "logic" cover + unreachable case b01.
By default SpinalHDL only checks "logic" coverage. To reach full "bits" coverage, unreachable cases are redirected to the last case of the switch
(note: it can be an is
as well as a default
?).
If you have defined is
cases with a complete "logic" cover and want to define a specific behavior for unreachable cases, you will add a default
case; but as all reachable cases were already covered, it will be considered unreachable. To not consider it unreachable and use it for unreachable cases, use coverUnreachable = true
.
Example…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so it matters if I use an enumerate (logic cover) or just bits (bits cover) [regardless of constants or immediate values].
I will try to rewrite it then to make it a bit more close to that understanding. Maybe I can also come up with a good example to code show this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it is just my understanding and I do not use it so maybe my understanding is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by default, will handle the uncovered cases with the last
is
block.
That sound wrong or else confusing to me.
The goal here is to document coverUnreachable right ?
What's about something kind of like :
"""
If a switch/is contains a 'default' statement while already all the possible values of the 'switch' are covered by the 'is' values, SpinalHDL will generate an "UNREACHABLE DEFAULT STATEMENT" error. You can drop this error reporting by specifying switch(myValue, coverUnreachable = true){ ... }.
"""
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dolu1990 was my suggestion correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say, this should not be documented, as it is mostly a generation artefact, not realy a feature.
I 80% agree so I agree XD
Also, notes SpinalHDL replace the last "is" with a "default" only if all the logical values are covered by the "is" statements
Okay now I 100% agree to just don't tell about it then XD XD
So for the explanation of coverUnreachable
, I suggest:
- Explain logic vs bits coverage
- Mention that to manage bits coverage a user-defined way (do not mention what Spinal does by default) if all logic coverage is done with only
is
statements, have to usecoverUnreachable
with adefault
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will adapt it then according to what I see here. I couldnt follow this in the past few days but I will catch up and propose a respective change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@numero-744 Seems good to me ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So i had time now to read a bit here, what I conclude is the documentation wont talk about all of the details we discussed about here but what @numero-744 concluded "show and explain logic(enum?) vs bits" and the small thing about coverage of all logic cases by using coverUnreachable for some corner cases of switch-is
Co-authored-by: Côme <[email protected]>
Co-authored-by: Côme <[email protected]>
Co-authored-by: Côme <[email protected]>
Co-authored-by: Côme <[email protected]>
Co-authored-by: Côme <[email protected]>
Co-authored-by: Côme <[email protected]>
I pushed some change. I'm sorry, i was wrong about the strict option. It is a option to remove duplication inside a is statement, not between is statements XD let's me know if all is good for you with the current state of the PR. |
Works for me |
Co-authored-by: Côme <[email protected]>
Thanks @saahm @numero-744 :D |
For #138 discussed in SpinalHDL/SpinalHDL#946
also added documentation for the strict option since its part of the same structure and would belong in the same place.
I am open for slightly less technical/deep descriptions of these parameters as I dont know if it would confuse anyone reading that first time.