-
Notifications
You must be signed in to change notification settings - Fork 60
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
first PYUVM Register Model release and Regression Fix #158
Conversation
Regression based on "make test" command Results (0.06s): |
The regression tests are failing. |
I see that there are many flake8 errors. These are pretty easy to fix, especially in vscode. |
Is there any other command I could run along with the make test ? Sorry I thought erroneously that make test was the only sanity there in the directory. What is the best chosen approach to use for the sanity ? Thanks |
`tox` will run all the tests:
```
% cd github_pyuvm
% git remote -v
% git status
% git pull
% git status
% tox
```
…On Sun, Aug 6, 2023 at 9:08 AM EngRaff92 ***@***.***> wrote:
Is there any other command I could run along with the make test ? Sorry I
thought erroneously that make test was the only sanity there in the
directory. What is the best chosen approach to use for the sanity ? Thanks
—
Reply to this email directly, view it on GitHub
<#158 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYFXWSJTAQ3MNWJ2YYZKILXT6JNTANCNFSM6AAAAAA3F3DDAY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Great I’ll run again and try to fix probably the next delivery will have the full write read front door in place ready for the RTL. Unfortunately no back door. At least should be ready to be used publicly thanks |
Cocotb must provide a way for the backdoor. I'll investigate.
…On Sun, Aug 6, 2023 at 9:14 AM EngRaff92 ***@***.***> wrote:
Great I’ll run again and try to fix probably the next delivery will have
the full write read front door in place ready for the RTL. Unfortunately no
back door. At least should be ready to be used publicly thanks
—
Reply to this email directly, view it on GitHub
<#158 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYFXWUSUVEO36IWEXNSGC3XT6KB7ANCNFSM6AAAAAA3F3DDAY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I see why this is failing. To import pyuvm modules into other modules, you need to include
becomes
|
Fixed, I did make anyway a pyuvm_reg_pkg which should make life easier |
I just updated the testing tool to only use tox. There were inconsistencies between flake8 in our workspaces and GitHub. Please do a pull and try again. |
should I close this PULL request and do another one ? |
It's not necessary. You can pull the latest and then push the new commits into the pull request. The pull request is just a branch. |
I would need to ask Google how to do it. |
The latest Pull request has 2 commits in it already |
You can add as many commits as you like to a pull request before we
actually merge it.
…On Sun, Aug 6, 2023 at 11:10 AM EngRaff92 ***@***.***> wrote:
The latest Pull request has 2 commits in it already
—
Reply to this email directly, view it on GitHub
<#158 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYFXWVADBAX6MV36YU5L4LXT6XV7ANCNFSM6AAAAAA3F3DDAY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Yeah I think I will keep adding commits but I do see there are plenty failures on flake8 which would require a new formatting on quote few files. I'll work on that but that eventually could require a bit of time. Thanks |
We only check the pyuvm directory. If it helps.
…On Sun, Aug 6, 2023 at 11:17 AM EngRaff92 ***@***.***> wrote:
Yeah I think I will keep adding commits but I do see there are plenty
failures on flake8 which would require a new formatting on quote few files.
I'll work on that but that eventually could require a bit of time. Thanks
—
Reply to this email directly, view it on GitHub
<#158 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYFXWQYYAYU2OK3JYFGVBDXT6YPXANCNFSM6AAAAAA3F3DDAY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
IF you coordinate, I am happy to take some files to fix the flake8 issues. |
Oh yes that would definitely help out. But I do not wanna demand changing things. If required I’ll get all the files sorted out. Mainly up to you |
I am happy to work through the files and fix flake issues. I've checked out the pull request branch, so I could push my changes back to the pr branch. I just don't want to create a situation where we both edit the same file at the same time. Which files are you working on now? |
At moment I’m working on reg block and reg map mainly. At moment the main work is focused there. Thanks again |
I just fixed s19.... so that it would pass flake and pushed the commit. It took about 15 minutes, but will go faster. It is mostly regular expression replacements. I recommend using vscode with flake8 checking turned on for development. |
pyuvm/s21_pyuvm_reg_map.py
Outdated
# get_parent | ||
def get_parent(self): | ||
return self._parent | ||
|
||
# get_base_addr | ||
def get_base_addr(self): | ||
return self._base_addr | ||
|
||
# add_reg | ||
def add_reg(self, reg, offset): | ||
self._regs[offset] = reg | ||
|
||
# get_registers | ||
def get_registers(self): | ||
return list(self._regs.values()) | ||
|
||
# get_reg_by_offset | ||
def get_reg_by_offset(self, offset): | ||
return self._regs[offset] | ||
|
||
## set_predictor | ||
def set_predictor(self, predictor): | ||
if isinstance(predictor, pyuvm_reg_predictor): | ||
self.predictor = predictor | ||
else: | ||
error_out(self.header,"predictor should be type of uvm_reg_predictor") | ||
|
||
## get_predictor | ||
def get_predictor(self): | ||
if self.predictor == None: | ||
error_out(self.header,"predictor Not set") | ||
else: | ||
return self.predictor | ||
|
||
## set_adapter | ||
def set_adapter(self, adapter): | ||
if isinstance(adapter, pyuvm_reg_adapter): | ||
self.adapter = adapter |
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.
@EngRaff92 , I've updated s18_register_model.py
recently with changes to uvm_reg_map
(#157). Would it be possible to refactor some methods from there ?
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.
Absolutely which methods do you wanna refactor ?
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.
some instance variables from __init__()
and the following methods
__is_addr_aligned
add_reg
add_submap
set_sequencer
__is_map_valid
get_submap_offset
set_submap_offset
set_base_addr
reset
get_root_map
get_parent
get_parent_map
get_base_addr
get_n_bytes
most of the them are not conflicting except add_reg
, set_sequencer
I think
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.
Hey so basically since the initial RAL setup will only have one MAP I do believe that I will skip for the time being set_submap_offset and get_submap_offset is that ok for you ?
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.
Yes, makes sense. We could add it once the base RAL implementation is done
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.
@crteja and @EngRaff92 , I would like to let you take the lead on RAL. I have to prepare classes this month for school at the end of the month, but I am happy to handle simple tasks.
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.
Sure
Great I did not know there was an extension available for flake in vscode I will start using it thanks so much |
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.
Thanks @EngRaff92 , overall the changes look good. I've added comments for minor modifications.
pyuvm/s17_pyuvm_reg_enumerations.py
Outdated
@@ -4,17 +4,16 @@ | |||
from enum import Enum | |||
|
|||
|
|||
UVM_REG_DATA_WIDTH = 64 | |||
UVM_REG_ADDR_WIDTH = 64 | |||
PYUVM_REG_DATA_WIDTH = 64 |
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.
@EngRaff92 , I don't think these variable names conflict with any other variable in pyuvm
module.
Any reason for renaming these variables and other similar changes in this file ?
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.
Generally speaking the general thought I had was to replace uvm as pyuvm to be consistent with the library name. Was just an idea and all the suggestions are more then welcomed hence if you all disagree I will rename it back. Thanks
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.
It would be good to use one naming convention either pyuvm
or uvm
.
@raysalemi , what do you think ?
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.
We should match the 1800.2 IEEE spec or whatever is in the rest of pyuvm.
@@ -17,8 +17,19 @@ | |||
# Section 14, 15 (Done as fresh Python design) |
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 think it would be better to remove the _pyuvm_
in file names to keep it consistent with existing file naming convention. As it is not a functional change, we can take it up as part of a separate cleanup PR
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'm not sure what this refers to.
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.
sorry, linked to the wrong file. This comment refers to the newly added file names. Current files in repo doesn't has _uvm_
in the name, whereas the new files have _pyuvm_
. it would be good to use one naming convention either _pyuvm_
or _uvm_
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.
Yeah thinking about how many files do have uvm in the name rather then pyuvm best is to revert back mine to uvm. I’ll do it immediately so I can do a new commit today
pyuvm/s19_pyvum_reg_field.py
Outdated
from pyuvm.s20_pyuvm_reg import pyuvm_reg | ||
|
||
# Class declaration for register field | ||
#@rand_enable(enable_pyvsc) |
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.
nit: Please remove this comment if there is no plan to use this decorator
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.
There is a plan on using it once I manage to get pyvsc compiled on Mac (now broken)
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’ll add a todo
pyuvm/s19_pyvum_reg_field.py
Outdated
## Keep desired value as random | ||
if enable_pyvsc == True: | ||
pass | ||
#self._desidered = vsc.rand_bit_t(self._size) |
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.
nit: same as above
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.
These can all be flagged with TODO as you say below.
pyuvm/s19_pyvum_reg_field.py
Outdated
self._has_been_writ = False | ||
self._prediction = predict_t | ||
self._response = pyuvm_resp_t | ||
self._name = name |
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.
This is redundant as this class is derived from uvm_object
. name
can be retrieved using get_name
method
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.
Yes. In fact this would be an error if it is extending uvm_component
.
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.
Sorry did have a look at what was in the object I will then remove all the get name methods so
pyuvm/s20_pyuvm_reg.py
Outdated
self.throw_error_on_read = throw_error_on_read; | ||
self.throw_error_on_write = throw_error_on_write; |
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.
Are these instance variables redundant ? There seems to be no code using these in uvm_reg
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.
You do not see it using because of no test at moment performed for the read write. This will emulate the concept of send an error that usually register interfaces have. So the idea behind it is using those to control the expected error
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.
ok. makes sense.
pyuvm/s20_pyuvm_reg.py
Outdated
self.throw_error_on_write = throw_error_on_write; | ||
## Call the build function before adding any register to the main BLOCK | ||
self.build() | ||
## Check if the lock for the above register is set |
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.
Could you please add a TODO for the comments where more work needs to be done. This makes it easier to check later for all outstanding work.
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.
Sure I will and I agree with you
pyuvm/s18_pyuvm_reg_block.py
Outdated
#import vsc | ||
|
||
# 18.1.1 Class declaration | ||
class pyuvm_reg_block(uvm_object): |
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.
Would it be possible to extend all classes in this change from uvm_report_object
so that logging functionality is available via self.logger.<info|warn|error|debug>
@raysalemi , please correct me if this is not the case
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.
We need to follow the spec. What does the spec say?
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.
the uvm report object is coming from uvm object. The standard uses uvm object directly perhaps the uvm object could have a reference to the report logger. Frankly if we have to follow the spec not sure if using the report object would make sense
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 thought it would be easier to reuse the logging facilities of uvm_report_object
. This is primarily to avoid using print
.
Would you be able to do something like this to get logger handle in each of these new base classes
pyuvm/pyuvm/s06_reporting_classes.py
Lines 43 to 45 in 3ce8c97
# Every object gets its own logger | |
logger_name = self.get_full_name() + str(id(self)) | |
self.logger = uvm_root_logger.getChild(logger_name) |
and replace print
with self.logger.<info|warn|error|debug>
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.
The user can use pyuvm logging without the object being a descendent of uvm_report_object. We do that all the time in sequences.
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 ok. I've only seen logging using uvm_root().logger.info
. Its good that its already available.
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.
Yes. That is how to do it.
But, again, we must follow IEEE 1800.2 when it comes to inheritance.
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.
Agreed. Thanks
pyuvm/s25_pyuvm_adapter.py
Outdated
# the corresponding members from the given generic ~rw~ bus operation, then | ||
# return it. | ||
def reg2bus(rw: uvm_reg_bus_op): | ||
pass |
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.
Shouldn't this be raise <exception>
instead of pass
? User must implement reg_adapter
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.
Shouldn't this be
raise <exception>
instead ofpass
? User must implementreg_adapter
Yes. It should raise an exception unlesspass
is a reasonable behavior. There is also error_classes.UVMNotImplemented()
if you want a more descriptive exception.
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.
Correct should raise an exception is done at my end I just have to push it thanks for highlighting it
pyuvm/s25_pyuvm_adapter.py
Outdated
# is not allocated from scratch. This is to accommodate applications | ||
# where the bus response must be returned in the original request. | ||
def bus2reg(bus_item: uvm_sequence_item, rw: uvm_reg_bus_op): | ||
pass |
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.
same as above
Hey @crteja thanks so much for your review once back home I will try to resolve all of them. Thanks again |
Fine by me |
Sure yes that means this dev branch from my fork must be pushed there right ? Before I need to copy all of your changes in my folder locally so that I’m up to date, or is the branch already up to date with the latest commits in this PR? |
Thanks @raysalemi . |
Have you pushed your latest commits to |
I see the following steps to move to ral_dev:
Sound right? |
no, I just saw the |
I have created this discussion: #161 |
looks good to me |
|
Yes I can, sure, but unfortunately not now because I do not have access to my personal PC since I’m in the office. Since @raysalemi made some changes to fix flake8 errors I do have to firstly rebase and merge locally and then commit. If I do commit now there will be no merge I assume. Anyway I will do that for sure by eod. Thanks to everyone |
I am blessed with being on summer vacation. I am preparing for a high school class where we will build radio-controlled airplanes. |
That is so cool definitely something I would love exposing in my room 100%. Thanks for sharing. To be honest I have not seen so many cool project at school that class look like a lucky one |
Once we have all commits on the |
looks good to me |
Agre but once we will merge on the main branch a merge should be anyway required which would need us to review every file changes, am I mistaken? |
We don't have to review all the files for merge. As long as there are no changes in the main branch conflicting with RAL files, merge should be a simple step. |
Not sure to get you 100%. To make sure there are no changes in the main branch we should deliver not at same time and not the same files. As long as we modify the same files and assume we do deliver the same time even if is a minor change there should be conflict to resolve. On top of that having 3 separate branch means we do not know what has been changed till we pull the main right ? |
Sorry if I confused you, I meant for the final merge to main branch there is no review required. For each PR in to |
No bother at all now I get you, thanks |
@EngRaff92 , I've added a comment with list of things in #161 .We can continue the discussion there. |
Eventually |
yes, I don't know whether the merge of |
I always merge using pull requests, even for my own work. |
Please let me know when all the commits in this pull request have been transferred to |
There is some confusion here, I think. This PR should now be closed. |
@EngRaff92 These changes should go to |
First PYUVM Register Model release
pyuvm/s18_pyuvm_reg_block.py
delete mode 100644 pyuvm/s18_register_model.py
create mode 100644 pyuvm/s19_pyvum_reg_field.py
create mode 100644 pyuvm/s20_pyuvm_reg.py
create mode 100644 pyuvm/s21_pyuvm_reg_map.py
create mode 100644 pyuvm/s22_pyuvm_mem.py
create mode 100644 pyuvm/s23_pyuvm_reg_item.py
create mode 100644 pyuvm/s24_pyuvm_reg_includes.py
create mode 100644 pyuvm/s25_pyuvm_adapter.py
create mode 100644 pyuvm/s26_pyuvm_predictor.py
create mode 100644 pyuvm/s27_pyuvm_reg_pkg.py
delete mode 100644 tests/pytests/test_18_register_model.py
create mode 100755 tests/pytests/test_pyuvm_ral.py
create mode 100755 tests/pytests/test_pyuvm_reg.py
create mode 100755 tests/pytests/test_pyuvm_reg_block.py
create mode 100755 tests/pytests/test_pyuvm_reg_field.py
create mode 100755 tests/pytests/test_pyuvm_reg_map.py