-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add emulator support #5
base: master
Are you sure you want to change the base?
Add emulator support #5
Conversation
c84c302
to
dbf3dc5
Compare
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.
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. |
2e15db8
to
e5a8fdb
Compare
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? It's called once in __init__() and once inside open(), which is not called if emulator mode is enabled.
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.
yup, you are right
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.
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() |
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.
yup, you are right
39bae6a
to
1047e8a
Compare
Codacy analysis failures are of low severity. However, they should be dealt with in a future PR of their own. |
1047e8a
to
7ed52ec
Compare
LGTM 👍 |
f3b5954
to
415bea0
Compare
2f4c1c7
to
3043a9f
Compare
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
f54902f
to
d3ddc89
Compare
it in the BpodBase class