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

Code Reorganization Enhancement #446

Open
DJShepherd opened this issue Jun 12, 2023 · 4 comments
Open

Code Reorganization Enhancement #446

DJShepherd opened this issue Jun 12, 2023 · 4 comments

Comments

@DJShepherd
Copy link
Contributor

DJShepherd commented Jun 12, 2023

As I've been working on cleaning up and refactoring some of the API, I've noticed a couple of things that could be beneficial to the code base. I am open to input if any of this seems desirable!

  • Split classes (and related classes) into their own files. For example, ChipWhispererExtra has a handful of various classes. Ideally, it'd be nice to split them into their own file so it's easier to read through, find code, etc.
  • Refactor interfaces with proper hierarchies for different products. For example, have a base GPIOSettings class and make GPIOSettingsCWLite, GPIOSettingsHusky, etc etc. Obviously, hardware specific functionality gets overriden and you don't hafta test for HW support since it's inheritantly designed into the target HW interface. And then during scope object creation, when you detect what device it is, you create all the necessary HW specific interface objects. There seems to be some objects that are structured like this (ex TriggerSettings, etc), but there are other designs that use things like _is_husky, or check the HW string, etc. Would be cleaner to bring it all to a consistent design.

Of course, I am offering to do this work! I'd work on it after I finish refactoring the current stuff I'm working on. Any thoughts?

@alex-dewar
Copy link
Contributor

Hi,

Thanks for the input on this.

Split classes (and related classes) into their own files. For example, ChipWhispererExtra has a handful of various classes. Ideally, it'd be nice to split them into their own file so it's easier to read through, find code, etc.

I agree that this is a good idea. Perhaps folders for all the different cw modules (io, adc, glitch, etc.) with files in them for each scopetype (husky_io.py, lite_io.py, etc.)? I do think it's easy to go too far here though - I personally find jumping between files to be a lot more jarring than jumping around a single file.

Refactor interfaces with proper hierarchies for different products. For example, have a base GPIOSettings class and make GPIOSettingsCWLite, GPIOSettingsHusky, etc etc. Obviously, hardware specific functionality gets overriden and you don't hafta test for HW support since it's inheritantly designed into the target HW interface. And then during scope object creation, when you detect what device it is, you create all the necessary HW specific interface objects. There seems to be some objects that are structured like this (ex TriggerSettings, etc), but there are other designs that use things like _is_husky, or check the HW string, etc. Would be cleaner to bring it all to a consistent design.

Yeah, extending similar classes is probably the "correct" way to do different hardware like this. The other methods are a little easier I think when adding the first parts of new functionality, which is probably why a lot of the CW code is structured like that. I think consistency is probably the best thing to aim for here. It also works better with how the documentation is setup currently.

This might be best done by code similarity. For example, a lot of things on the Pro are just extensions of things on the Lite, so it makes sense to have those modules be extensions of the CWLite modules. Conversely, the Nano functions very differently from the Lite, so it probably makes sense for a lot of that to be completely separate classes.

I'm not really sure if base classes make much sense with Python's duck typing. In a lot of languages, you can use it to define required functionality for a certain type of object (i.e. a scope object needs a capture() method, but this isn't necessary at all in Python. I might take a looks around and see how other projects end up doing things like this.

We do have a training coming up (beginning of August), so we might also not look too much at it until that's over.

Alex

@DJShepherd
Copy link
Contributor Author

I agree that this is a good idea. Perhaps folders for all the different cw modules (io, adc, glitch, etc.) with files in them for each scopetype (husky_io.py, lite_io.py, etc.)? I do think it's easy to go too far here though - I personally find jumping between files to be a lot more jarring than jumping around a single file.

I also agree with the "too far" sentiment. I think it would be beneficial to breakout the different modules, as suggested, into their own files (GPIOSettings, TriggerSettings, etc), but from there, I have 2 ideas on how to organize HW implementations:

  • In the proposed new breakout file, have all the HW specific implementations in the same new breakout file. Ex: GPIOSettings, GPIOSettingsCWLite, GPIOSettingsHusky, etc would all reside in CWGPIOSettings.py (or whatever you would want it to be named)
  • Or, have just the base and shared/common implementations in the new breakout file and create (or utilize an existing) HW specific file to have all HW specific classes inside it. Ex: GPIOSettings would be in CWGPIOSettings.py, while ScopeCWLite, GPIOSettingsCWLite, TriggerSettingsCWLite, etc would be in CWLite.py, or whatever naming/organizational method is desirable.

From my observations, most HW specific overrides/additions are small enough and few that a separate file isn't necessary so regardless of where these implementations are, it wouldn't add any significant bloat.

I think I would personally go with option 2. Keeps the base class files clean and allows you to work with 1 file for the specific HW you are working on.

@alex-dewar
Copy link
Contributor

Hi,

Just wanted to let you know that we're back from Blackhat and I'll be going through your pull requests again.

Thanks for your patience on this.

@alex-dewar
Copy link
Contributor

Also to comment on your suggestion above:

I'm not too sure what the best option is here. I'd kinda lean towards option 1, just because I don't find myself hopping between different modules too often. Either way would be a big organizational improvement through - just being able to go scopes/(modules?)/io/CWIOSettings.py or something vs. scopes/cwhardware/ChipWhispererExtra.py is way better.

One suggestion I'd make is that CWNano stuff should go in its own file (NanoIOSettings.py), since all the Nano stuff is very different/already separated from the FPGA based capture boards.

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

No branches or pull requests

2 participants