lowRISC / style-guides

lowRISC Style Guides
Creative Commons Attribution 4.0 International
359 stars 122 forks source link

[dv/style] Handling EOT with phase_ready_to_end #37

Closed udinator closed 4 years ago

udinator commented 4 years ago

This PR attempts to encapsulate the discussion around end-of-test practices (namely using phase_ready_to_end()) in #30 and add it to the DV style guide.

The code example of phase_ready_to_end() is taken from lowrisc/opentitan#2137.

This PR also contains some very minor changes to get rid of the word you which is used in several locations.

Signed-off-by: Udi Jonnalagadda udij@google.com

udinator commented 4 years ago

The force push deletes the section about alternate timeout mechanisms, and changes the word function to method when referring to phase_ready_to_end().

rasmus-madsen commented 4 years ago

LGTM I'm ok with you merging

udinator commented 4 years ago

Hi Udi! Thanks for doing this, and I realise that it's taken quite a while. I've got a few more-or-less nitty comments below, which I think should probably be fixed before this goes in.

The general thing that I think needs fixing (addressed by many of the specific comments below) is that we've got a sort of "half-dv_lib" recommendation at the moment: the text is sort of explaining how you might rewrite dv_lib and assumes that you'll have a monitor base class etc. I don't think that's quite right.

If we want this to be free-standing, and not talk about our DV libraries, I think the right thing is to say "A monitor should do XYZ" and leave it to the reader to work out how the class hierarchy works. Maybe we should add a note somewhere in the text saying that dv_lib exists and that you get some of this functionality for free by using it?

I agree, looking back at this there seems to be some wording that implies there is some parent monitor class that everything inherits from. I've gone through and changed the wording of the comments to make it more explicitly 'free-standing'. Thanks for the comments!

udinator commented 4 years ago

@weicaiyang would you mind taking a look at this just to make sure it aligns with the code in your OT PR?

udinator commented 4 years ago

Thanks @weicaiyang !