Skip to content
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

Move lock methods to base class #23

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

toddstrader
Copy link
Contributor

Move generically useful lock methods into Driver base class.

@ktbarrett
Copy link
Member

What is the purpose of the lock here?

@toddstrader
Copy link
Contributor Author

What is the purpose of the lock here?

So that any driver can safely have multiple coroutines driving without running into each other just like the avalon and opb drivers already do.

@ktbarrett
Copy link
Member

That's what I figured. Are they meant to be directly used by the user? If so why are the methods private (starting _)? If not, I think you might need to update some other code in the base class to use the lock.

There is also cocotb.triggers.Lock... but that could be a different PR.

@toddstrader
Copy link
Contributor Author

True. We have a fork with this change and derived Driver are using these method (seemingly without issues). I'll take a closer look to see why it's private-ish.

Separate question: why not use "__" for making methods private?

@ktbarrett
Copy link
Member

ktbarrett commented Apr 22, 2021

In Python using _name is "private" by convention, you can still access it if you need to. __name does name mangling so only instances of the class it's created in can access it. This is done for class-private values, things not intended to be used in subclasses. Subclasses here are intended to use private methods in the base classes. By "user" I meant the person using the driver. It seem like this should be used in the send method, but I didn't see you update the send method of the base class to use it. That's the point I was trying to make.

@toddstrader
Copy link
Contributor Author

Yeah, so I guess to elaborate on what I'm trying to do: I'd like to be able to have all drivers choose to acquire a lock like avalon and opb do in their read/write methods. I didn't put it in send because those guys don't appear to be using send (unless I'm missing something). I could also add it to send if you think that's useful. I'd probably surround this line in _send with the lock:

        await self._driver_send(transaction, sync=sync, **kwargs)

And yes, sorry for the confusion. I have no intention of users using these methods, just derived Drivers.

@ktbarrett
Copy link
Member

I guess we don't have to worry about it at this point. It might somehow break people.

@ktbarrett
Copy link
Member

ktbarrett commented Apr 22, 2021

I'll wait a bit to see if there are any other opinions and merge it in a couple days otherwise.

@ktbarrett ktbarrett merged commit a3e22f7 into cocotb:master Apr 26, 2021
@toddstrader toddstrader deleted the lock_refactor branch April 26, 2021 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants