Conversation
ckaraneen
force-pushed
the
add-emulator-support
branch
3 times, most recently
from
c84c302 to
dbf3dc5
Compare
oraby
left a comment
oraby
left a comment
There was a problem hiding this comment.
I've had a go through the code except for the actual emulator class, but I wanted to give some initial feedback first. Generally, I think the code is doing a good job where it basically satisfies what needs to be done, with the exception of few unrelated cosmetic changes.
I have wished though if the emulator was implemented as an instance of a BpodCOMProtocol() class, however that's not currently possible as the current Bpod classes design uses inheritance when at many times composition would have been IMO a better choice.
One last thought: I wonder whether it'll be simpler to replace emulator.enabled with simply having self._emulator either as Emulator() or None.
I agree. Regarding CI: If these tests will be part of CI, your aforementioned comments are true, and we 'll also have a problem with the threaded emulator examples, where I showcase the manual override functionality. But, these tests were intended to demonstrate emulator functionality in the spirit of the examples under the function_examples and state_machine_examples directories. All these are not unit tests. All in all, I think that CI should not be configured to run these behavioral tests, but only unit tests, if they are added in the future (separate PR), which I totally recommend. |
ckaraneen
force-pushed
the
add-emulator-support
branch
10 times, most recently
from
2e15db8 to
e5a8fdb
Compare
oraby
left a comment
oraby
left a comment
There was a problem hiding this comment.
Generally LGTM, I've left few comments on the parts that I think still need addressing. But nothing major on my side.
examples/emulator_examples/global_counter_example_manual_override.py
Outdated
Show resolved
Hide resolved
| # initialise the thread that will handle the stdin commands | ||
| self.stdin = NonBlockingStreamReader(sys.stdin) if settings.PYBPOD_API_ACCEPT_STDIN else None | ||
| ##################################################### | ||
| self.__initialize_input_command_handler() |
There was a problem hiding this comment.
If this is emulator mode, then this function would be called twice, once here and once in e652786#diff-c1bf0eb6016fc3d3327d1ee1ddb66991R75
There was a problem hiding this comment.
What do you mean? It's called once in __init__() and once inside open(), which is not called if emulator mode is enabled.
oraby
left a comment
oraby
left a comment
There was a problem hiding this comment.
One very simple change suggested, otherwise LGTM.
The only thing that I might also suggest, is to eventually squash together commits that are created then fixed in this PR into the same original initial commit.
| # initialise the thread that will handle the stdin commands | ||
| self.stdin = NonBlockingStreamReader(sys.stdin) if settings.PYBPOD_API_ACCEPT_STDIN else None | ||
| ##################################################### | ||
| self.__initialize_input_command_handler() |
ckaraneen
force-pushed
the
add-emulator-support
branch
from
39bae6a to
1047e8a
Compare
|
Codacy analysis failures are of low severity. However, they should be dealt with in a future PR of their own. |
ckaraneen
force-pushed
the
add-emulator-support
branch
from
1047e8a to
7ed52ec
Compare
|
LGTM 👍 |
ckaraneen
force-pushed
the
add-emulator-support
branch
2 times, most recently
from
f3b5954 to
415bea0
Compare
ckaraneen
force-pushed
the
add-emulator-support
branch
from
2f4c1c7 to
3043a9f
Compare
ckaraneen
force-pushed
the
add-emulator-support
branch
2 times, most recently
from
a92dc05 to
3d215d1
Compare
- Add Emulator object for emulating bpod hardware behavior and communicating outputs to emulator GUI server - Modify bpod classes to accommodate emulator functionality - Add emulator examples to showcase its usage
ckaraneen
force-pushed
the
add-emulator-support
branch
from
f54902f to
d3ddc89
Compare
2 participants
it in the BpodBase class