-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add adapter module for bus #566
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.
Proper AXI tests left for later.
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.
Only few small comments
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 fix the docstrings? I've pointed out some issues, but there is more. Also, is using a Protocol
for the bus configurations justified?
class BusParametersInterface(Protocol): | ||
""" | ||
Bus Parameters Interface for common buses | ||
|
||
Parameters | ||
---------- | ||
data_width : int | ||
An integer that describe the width of data for parametrized bus. | ||
addr_width : int | ||
An integer that describe the width of address for parametrized bus. | ||
granularity : int | ||
An integer that describe the granularity of accesses for parametrized bus. | ||
""" | ||
|
||
data_width: int | ||
addr_width: int | ||
granularity: int |
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.
Is the use of a Protocol
necessary here? Shouldn't this just become a dataclass?
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.
If it would be dataclass
then all specific buses implementations will have to use it for holding parameters. Presumably some bus can have different parameters than other buses and then that dataclass
will have to be extended and in some cases would have redundant fields. Please correct me if I am missing something.
With Protocol
we specify parameters that are important from Common Bus Master side point of view.
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.
Let's keep it the way it is for now.
There are conflicts to be resolved. |
Resolves: #531