feat: summary report more dashboard like #58
Conversation
machshev
requested review from
hcallahan-lowrisc,
martin-velay,
nasahlpa and
rswarbrick
src/dvsim/report/data.py
Outdated
|
|
||
| code: float | None = None | ||
| """Code Coverage.""" | ||
| toggle: float | None = None |
There was a problem hiding this comment.
I am surprised to see toggle coverage being not included inside code cov. Can you tell me the reason?
There was a problem hiding this comment.
Good question, I don't know the answer... I'm not a DV expert. Just replicated the logic for the existing OT dashboard https://opentitan.org/dashboard/index.html
Maybe we can discuss this Monday on a call and you can explain?
There was a problem hiding this comment.
I see, that's very surprising on the OT dashboard too! I don't know if there is a particular need for that, but it shouldn't be separated. That's also probably something we should change on OT board.
Do you know who has done that on OT board? If yes, can you tag him/her in this thread or talk directly with this person?
There was a problem hiding this comment.
src/dvsim/tool/vcs.py
Outdated
| "Sequence[float]", | ||
| [ | ||
| raw_metrics[m] | ||
| for m in ( |
There was a problem hiding this comment.
That's where you should have the "toggle".
src/dvsim/tool/xcelium.py
Outdated
| "Sequence[float]", | ||
| [ | ||
| raw_metrics[m] | ||
| for m in ( |
There was a problem hiding this comment.
I guess we should also see toggle here too.
src/dvsim/tool/xcelium.py
Outdated
| "Sequence[float]", | ||
| [ | ||
| raw_metrics[m] | ||
| for m in ( |
There was a problem hiding this comment.
I don't really know if it matters, but maybe good to write it somewhere or at least for your own knowledge. Xcelium and VCS use different words for almost the same things:
| Coverage Concept | Synopsys VCS Term | Cadence Xcelium Term |
|---|---|---|
| Did this part of code execute? | Line (or Statement) Coverage | Block Coverage |
| Did this if/case take all paths? | Branch Coverage | Branch Coverage |
| Did the logic evaluate to 0 & 1? | Condition Coverage | Expression Coverage |
| Did the signal wiggle? | Toggle Coverage | Toggle Coverage |
| Did the state machine transition? | FSM Coverage | FSM Coverage |
The main difference and confusion is about line vs block:
Cadence block: counts blocks of code. A "block" is a sequence of statements between branching points (like begin and end). If the simulator enters a begin...end block, Xcelium assumes all statements inside that block are executed because there is no way to exit the block halfway through (unless there is a disable/return). Xcelium can also do statement (=line) coverage (counting individual statements), but "Block" is their primary metric.
There was a problem hiding this comment.
Thanks, this is really helpful information. We should probably have that as part of the documentation within the code base. Either a docstring in the EDA tool plug-in module or perhaps a README?
machshev
force-pushed
the
dashboard
branch
2 times, most recently
from
3416d7b to
5bac0d9
Compare
machshev
force-pushed
the
dashboard
branch
5 times, most recently
from
39ef96f to
501b9b2
Compare
|
@martin-velay I've updated the design as per our discussion. The underlying data model now preserves the raw data more accurately using the mapping you provided above in the table. Both the block and summary templates have been updated to display the information. 
|
machshev
force-pushed
the
dashboard
branch
2 times, most recently
from
33c2120 to
33791b1
Compare
|
After further feedback the headings are now hierarchical: |
Introduce a CoverageMetrics model instead of the dict. Create a new method on the tool plugins to convert from eda tool specific coverage fields to generic names that match the opentitan-site dashboard fields. Update the report summary and block report templates to use the new model. Add the extra fields in the summary report to make it more like the dashboard (contain the same information). Signed-off-by: James McCorrie <james.mccorrie@lowrisc.org>
Using color instead of table borders.
|
martin-velay
left a comment
martin-velay
left a comment
There was a problem hiding this comment.
OK, it looks much better and to go in the right direction. Let's approve that one and see if any further work is required. Thanks James!
2 participants
Introduce a CoverageMetrics model instead of the dict. Create a new method on the tool plugins to convert from eda tool specific coverage fields to generic names that match the opentitan-site dashboard fields.
Update the report summary and block report templates to use the new model. Add the extra fields in the summary report to make it more like the dashboard (contain the same information).
Fixes: #20