newaetech / chipwhisperer

ChipWhisperer - the complete open-source toolchain for side-channel power analysis and glitching attacks
http://chipwhisperer.com
Other
1.09k stars 280 forks source link

Code Reorganization Enhancement #446

Open DJShepherd opened 1 year ago

DJShepherd commented 1 year ago

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!

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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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.