Suggestion to make connect_cwlite_simpleserial.py more understandable

[rogerclarkmelbourne]

I'm not sure if you want suggestions as issues, but here goes...

I think connect_cwlite_simpleserial.py is confusing, because there is nothing in it which explains that the default target as defined in init.py is

from chipwhisperer.capture.targets.SimpleSerial import SimpleSerial as cwtarget

Hence is better if this script is to connect to the Simple Serial target, that you define

from chipwhisperer.capture.targets.SimpleSerial import SimpleSerial as cwSimpleSerialtarget

In this script and then pass cwSimpleSerialtarget as the type argument, which is currently getting defaulted to cwTarget in the definition of cw.target

e.g.

[code]import chipwhisperer as cw

scope = cw.scope()
self.scope = scope

from chipwhisperer.capture.targets.SimpleSerial import SimpleSerial as cwSimpleSerialtarget

target = cw.target(scope,cwSimpleSerialtarget)
self.target = target[/code]

Then its totally clear what type of target you are connecting to

I'm not a python expert, but I made this changed and tested the code and as far as I can tell it works the same as it did before, but its far more readable / self explanatory,

Or alternative consider adding some comments ;-)

[colinoflynn]

FWIW - the whole "init.py" thing is a shim to fix the API. Basically the idea of using CW 4.0 is that we wanted to get rid of the "insane" imports (like chipwhisperer.capture.targets.SimpleSerial) and have more stuff at the main level, which is more pythonic.

But that could be done with the current init.py (or with some minor changes) to make it more clear what is being connected (cw.target_simpleserial or cw.target.simpleserial or something). Basically we are trying to make the API how we want it for now (what is being done inside of init.py) & will eventually fix the "backend code".

[rogerclarkmelbourne]

EDIT.

I removed my previous comment as I totally miss understood how the XMEGA firmware worked

I had a quick look at the XMEGA firmware and I presumed that the command system would allow it to switch encryption modes, but I now see that it needs different firmware for each encryption type.

Getting back to the connection script.

I'm not an expert in Python code style practices.

I just find it odd that the default argument is taken from a global, and that global is given a generic name. But perhaps thats normal in the Python world.

[colinoflynn]

Ah, you mean the "target" or "scope" objects right?

They are actually in the command-line interpreter only scope, but all those scripts are run in that scope.

There's a bit of a trade-off with the scripting system - we try to simplify regular things (like accessing the scope/target objects), without hiding too much. The previous version had lots of "hidden" functionality which made this much worse in practice.

[rogerclarkmelbourne]

I mean cwtarget which is defined in init.py

I think you may still be hiding too much.

perhaps define

from chipwhisperer.capture.targets.SimpleSerial import SimpleSerial as cwSimpleSerialTarget

in init.py

and also change the cw object target function second argument so it does not have a default value. And then pass the cwSimpleSerialTarget e.g.

cw.target(scope,cwSimpleSerialTarget)

Otherwise anyone trying to work out how to set the target, needs to look at the definition for the target function, and then work out where the cwTarget object gets defined and what it is

[x8-999-github]

It Indeed it quite confusing that there is a default target.

[rogerclarkmelbourne]

@x8-999-github

Thanks... I'm glad its not me that thinks its confusing

[x8-999-github]

@rogerclarkmelbourne perhaps you can create a pull request to fix the issue? keeping this issue open will not change much

[rogerclarkmelbourne]

I'm not sure its worth doing a PR. There are 5 open PR's dating as far back as 2016.

I posted an issue asking why the existing PR's hadn't been either merged or closed, and was basically told that if I wanted a version of the repo with PR's merged, I'd have to fork and merge them in my own copy.

(I can't seem to find the PR in question, but unless I've very mistaken that was the general response)

[colinoflynn]

There are 5 open, but 41 closed (merged mostly). Some PR aren't rejected outright as the idea is reasonable, but needed more testing or had implementation issues. That is why they get left open & won't be merged to the core repo. Hence if you'd like any of the features that we are unsure about we leave them open for someone to merge to their copy, normally we figure out if it's worth closing them sooner but sometimes they get left in the TODO open state like you see. I'd rather that than just closing them w/o consideration and letting other people see them.

[alex-dewar]

This should hopefully be much more clear for CW5. Default arguments are left as is for cw.scope and cw.target since they're by far the most common options, but different options are easier to access (cw.scopes.whatever and cw.targets.whatever). This should also be better documented in the help() for the ChipWhisperer module, the cw.scope and cw.target functions, as well as in the first tutorial in the new Notebooks

[alex-dewar]

The stuff about where scopes/targets are coming from is better documented now, so closing.