Fix pytest

[kralka]

Part of issue #38

[jmichelp]

OpenADC is the scope that is embedded on the Chipwhisperer board.
I don't think it's a good idea to inherit from it.

They used to have a BaseScope class, implementing the basic API that any scope is expected to have to work with their software stack (con(), dis(), arm(), capture(), get_last_trace(), __str__(), __dict_repr__()) but it seems it's gone.
So we'll have to revive it on our side :(

[jmichelp]

See my previous comment.

It seems we'll need to define our own abstract class with the API that any scope should implement to work with our capture pipeline and have a class that makes OpenADC compatible with it (we don't care about CWNano in our case)

[jmichelp]

Don't derive from ChipwhispererCommonInterface. The name is a bit misleading but if you look at the methods it has, they aren't related to the scope at all (firmware updates, etc.).

We should simply get our class, then have a ChipwhispererScope that derives from ScopeTemplate and OpenADC

[kralka]

I am a bit lost here. I don't understand where a ChipwhispererScope that derives from ScopeTemplate and OpenADC comes into.

On the cw side: OpenADC derives from ChipwhispererCommonInterface [1]. And cw.capture_trace argument scope [2] has type ScopeTypes = Union[OpenADC, CWNano] (this is why I was afraid that mypy would consider passing any other type to be an error).

On scaaml side: We have only context managers (CWScope, DefaultCWScope, PicoScope) which have .scope property. And a single scope class acting as a ScopeTypes (Pico6424E).

[jmichelp]

Why is the Picoscope inheriting from OpenADC? It should only inherit from ScopeTemplate.

And we should have a class, for example ChipwhispererScope which inherits from both ScopeTemplate and OpenADC. This class being used to get the scope in our API

[jmichelp]

This should be typed a Optional[ScopeTemplate] or just ScopeTemplate because we may want the scope to be mandatory maybe?

[jmichelp]

Same thing here regarding prototype. I don't see what poll_done would bring. Removing it from the function signature and putting the default value in Chipwhisperer.capture() should do the trick for us. It's not a value we change.
If later we need extra arguments to support a specific scope, we can revisit it by adding **kwargs to the function.

Please document return value.

[kralka]

Same error with incompatible definition. ChipwhispererScope inherits both OpenADC and ScopeTemplate thus the base classes must have same signatures. Am I not understanding something?

[jmichelp]

I think that for the abstraction, def con(self, device_identifier: Optional[str] = None, **kwargs) should be an appropriate signature.
device_identifier being a way to disambiguate which scope we want it case multiple scopes from the same model are plugged id and detected. Default behavior when set to None is to connect to the first one found.
You can decide to either put **kwargs as part of the signature or simply remove it because we don't need it at the moment for the scopes we support. When we'll need it it will be easy to add it anyway.

This means that we'll have to implement def con() for ChipwhispererScope so that it passes on the correct default values to super().con().

[kralka]

With your signature I am getting error: Definition of "con" in base class "ScopeTemplate" is incompatible with definition in base class "OpenADC"

[jmichelp]

Argh. Forgot that Python doesn't support overloading functions.
This means we would have to remove the inheritance from OpenADC and wrap it instead:

class ChipwhispererScope(ScopeTemplate):
  def __init__(self):
    self._openadc: OpenADC = cw.scope()  # This always returns a scope or raises an exception IIRC

  def con(self, device_identifier: Optional[str]):
    self._openadc.con(sn=device_identifier, ...)

  def capture(self) -> bool:
    return self._openadc.capture(pool_done=False)

  def arm(self):
    self._openadc.arm()
...

Hopefully mypy won't complain about it somewhere else.

[kralka]

I think Python supports changing the signature when inheriting (Pico6424E.con signature is different from ScopeTemplate.con signature), but when doing multiple inheritance the signatures of superclasses must match. Other thing is that cw.capture_trace expects the argument scope to be of type Union[OpenADC, CWNano].

I do not understand the role of ChipwhispererScope in your last code snippet. We have a CWScope and DefaultCWScope context managers (auto-con/dis the OpenADC) and then we have a Pico6424E which plays a role of an object that behaves as an OpenADC (and is passed to cw API instead of OpenADC), then we have a context-manager with PicoScope.scope is Pico6424E. Finally we have reimplemented ScopeTemplate which is an abstract class capturing the cw scope interface.

Over all I agree with your comments in the sense of not polluting our API with unnecessary compatibility with CW. The thing is that I am not sure how to do that and still have a working typechecking. And I also agree that typechecking is far from perfect in the current PR (my idea was fix the workflows and then slowly improve the rest).

[kralka]

I suggest to resolve the inheritance question in another PR (issue: #79)

[invernizzi]

Thank you for all the changes, Karel!