-
Notifications
You must be signed in to change notification settings - Fork 60
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
[Python package] Clearer code? #373
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #373 +/- ##
==========================================
+ Coverage 48.93% 49.34% +0.41%
==========================================
Files 113 115 +2
Lines 10854 11010 +156
==========================================
+ Hits 5311 5433 +122
- Misses 5543 5577 +34
☔ View full report in Codecov by Sentry. |
b0e632e
to
260ac37
Compare
aabb9c8
to
79c810c
Compare
…rface) and a Notifier (dispatching event from IOObjet or QtObject to the Display)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More comments especially at the top of classes would help a lot
cd1476b
to
8b29543
Compare
Rationale
This PR does not simplify or restructure (much) the Python package code, but rather aims at highlighting module / class dependencies and showing off interfaces.
Some modules in the Speculos Python code has evolved fast lately, and without formal / explicit structure, it becomes hard to understand the code or the impact of changes.
I've started by typing the code, which is a huge help to explicit what's used where. In the meantime,
mypy
asserts my assumptions and helps revealing unexpected / strange things, circular dependencies and whatnot.Second step was to extract interfaces from the code in a neutral place (most in the existing
speculos.mcu.display
module, some other inspeculos.mcu.struct
module, and aspeculos.observer
module) and try to change the code so that it relies more on the abstractions rather than concrete classes (that's basically a dependency inversion task, which will help simplifying intricate code).I think the third step would be to then restructure the code so that the abstractions are stored in sensible place, and concrete classes are lighter and clearer. I'm not sure this step will be finished in this PR, as it may need more scrutiny and may take longer to merge than this one alone (and the bigger the PR, the more painful the rebasing).
Current changes are:
CI changes
flake8
(existed already), +mypy
andbandit
(this last is neutralized and will not redify the CI, although its output is interesting)Code
Typing
Adding as much typing hints as possible, slightly modifying the code to add clarity
Architecture
ObserverInterface
andBroadcastInterface
explicit an observer design pattern used through theAutomationServer
and theEventsBroadcaster
, and instantiated in themain
function. The first one is tagged as legacy, so I suggest it should be cleaned for the next stable version. For now, the new interface helps keeping track of the current usageGraphicalLibrary
interface for bothBagl
andNBGL
in order to ease their usage. Direct usage of eitherbagl
ornbgl
in theseproxyhal
module should not occur, they should be hidden inside thescreen
(aDisplay
implementation). It is not done currently, it may require more work, but at leastGraphicalLibrary
covers someFrameBuffer
methods, so that there is no longer code likescreen.nbgl.m.take_screenshot()
, but insteadscreen.gl.take_screenshot()
, withgl
being a generic type (GraphicalLibrary
, so eitherBagl
orNBGL
).Unfortunately, as some SephTags are NBGL-specifics, the dispatcher in
SeProxyHal.can_read
usesassert isinstance(screen.gl, NBGL)
which degrades a bit the abstraction.Display
interface is used byTextScreen
,Headless
andScreen
, however these implementation are not used consistently as the latter is shadowed byQtScreen
which usesScreen
through indirect composition.mypy
clearly sees the issue, however the code is quite intricate here so not sure I'll flatten the dependencies here. => this has been answered in the last bullet of this listDisplay
interface also implements a sort of unclear observer design pattern too through itsnotifiers
property and methods. I've started to define an interfaceIODevice
for these notified classes (often defined asklass
), they seem to be related to instances stored intoServerArgs
but I've not yet catched the meaning of this.Display
shenanigans, there is a very unclear way of managing the application graphic interfaces. There currently is 3 options:Text
,Headless
andQt
. The two firsts are quite similar, the last however, is very different and more complex. These classes are intertwined with the (now called)IODevice
objects, andQtObject
for the Qt one, with cross-dependencies all over the place (things like the notifiers andIODevice.can_read(klass)
). First step I found there was to cut these classes in two: aDisplayNotifier
, managing the event notifications, and aDisplay
, used by theIODevice
s to interract with the screen. This way, the interfaces are clear: themain.py
instantiates aDisplayNotifier
and run its loop, and thisDisplayNotifier
can internally instantiate the correctDisplay
and notify theIODevice
. This is to be improved yet, but still sounds better than previously, where theQtScreen
was really not on the same level as the others (Headless
andTextScreen
).