Conversation
.Adding polulu tic t825 stepper motor driver to works with omm_conditionner .Removing Start and stop string in graphical interface for juice pump to avoid omm_conditionner.py modification
…mm_conditionner (with one file per RFID card). .Added support for the RFIDRW-E-TTL RFID card from Priority 1 Design. .Integration of multi-card management code developed by Clement Yasar
.Thread support added in _polulu_tic_t825.py
supprimé : ideas.md modifié : opensesame_extensions/open_monkey_mind/open_monkey_mind/open_monkey_mind.ui modifié : opensesame_plugins/open_monkey_mind/omm_conditioner/conditioners/_polulu_tic_t825.py modifié : opensesame_plugins/open_monkey_mind/omm_conditioner/conditioners/_seed_dispenser.py modifié : opensesame_plugins/open_monkey_mind/omm_conditioner/omm_conditioner.py modifié : opensesame_plugins/open_monkey_mind/omm_detect_participant/__init__.py modifié : opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/__init__.py modifié : opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_base.py nouveau fichier : opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_lid650_665.py modifié : opensesame_plugins/open_monkey_mind/omm_detect_participant/omm_detect_participant.py modifié : pyproject.toml opensesame_extensions/open_monkey_mind/open_monkey_mind/open_monkey_mind.ui Added support for RfidRWeTTL and RfidLID650_665 boards opensesame_plugins/open_monkey_mind/omm_conditioner/conditioners/_polulu_tic_t825.py Improved Raspberry Pi platform detection (more compatible) Now also used to turn ON/OFF the sound amplifier on the custom board opensesame_plugins/open_monkey_mind/omm_conditioner/conditioners/_seed_dispenser.py Since N. Claidiere uses Linux, set the default port to /dev/ttyConditioner opensesame_plugins/open_monkey_mind/omm_conditioner/omm_conditioner.py Since N. Claidiere uses Linux, set the default port to /dev/ttyRFID opensesame_plugins/open_monkey_mind/omm_detect_participant/__init__.py Added support for RfidLID650_665 board opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/__init__.py Added support for RfidLID650_665 board opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_base.py Added an error_queue to the _rfid_monitor method, allowing errors from this thread to be read (checked in the run method). Increased SERIAL_READ_TIMEOUT to reduce CPU usage. From my perspective, I don’t see any drawbacks to this change. Fixed the number of bytes read using RFID_LENGTH. Reading 18 bytes only worked when two reads in a row occurred. opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_lid650_665.py Added a specific _rfid_monitor implementation for the _rfid_lid650_665 board opensesame_plugins/open_monkey_mind/omm_detect_participant/omm_detect_participant.py Since N. Claidiere uses Linux, set the default port to /dev/ttyRFID
…cipant_error_queue' when reusing rfid process
opensesame_plugins/open_monkey_mind/omm_announce/omm_announce.py
Retrieve start/stop/secondes from UI to _conditionner properties
opensesame_plugins/open_monkey_mind/omm_conditioner/omm_conditioner.py
Add queue_error to rfidrwettl
opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_rfidrwettl.py
feat: change line_edit_secondes to spinbox
opensesame_plugins/open_monkey_mind/omm_conditioner/__init__.py
Add rfid_length,rfid_sep,serial_baudrate to user modifiable parameters
opensesame_plugins/open_monkey_mind/omm_detect_participant/__init__.py
opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_base.py
opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_lid650_665.py
opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_rfidrwettl.py
Add usefull GUI connection between boards combobox and parameters
opensesame_plugins/open_monkey_mind/omm_detect_participant/omm_detect_participant.py
… entrypoint and in experiments (Added a property in omm_announce to prevent the RFID process from being closed by "childs" experiment).
opensesame_plugins/open_monkey_mind/omm_announce/omm_announce.py
opensesame_plugins/open_monkey_mind/omm_detect_participant/detectors/_rfid_base.py
…D for that reader(used by omm.current_participant_changed)
|
Sorry, I messed up the test. There are actually some showstopper issues. I'll flag them in the comments. |
There was a problem hiding this comment.
Let's first make sure that this PR doesn't break anything, because right now it breaks the RFID detector. I left a few specific comments, but they apply more generally:
- Clean up the code so that there are no unnecessary empty lines and otherwise non-standard formatting that makes the code look messy. We don't need to polish it to perfection, but right now it's too much. A style checker like
ruffis very convenient for this purpose. - Don't use blanket
try: except:statements that cover big chunks of code. Isolate lines that may actually crash, and then wrap these in exception-specifictry: except:s. More generally, when using AI to write code, make sure it doesn't use these kinds of sloppy shortcuts. We don't need to build in all kinds of safeguards to make the code fail silently when something is wrong. Because nothing should be wrong, and when it is, we want to know! - Test the code! The missing import of
tracebackcan never have worked, which means this was blind-coded without testing. Always test! And if you can't, for example because you don't have a certain device, indicate clearly what wasn't tested.
When you've addressed the above, use osexp/test-rfid-reader.osexp to make sure the RFID reader isn't broken. (Or ask me to do it if you can't test yourself.)
Let's get to work! 💪
| """ | ||
|
|
||
| readers = [] | ||
| try: |
There was a problem hiding this comment.
This try: except: covers a lot of code. This makes it difficult to debug. Instead of having these blanket try: except: blocks, think about which lines can actually cause an error, and white kind of error, and then wrap these exception-specific try: except: blocks.
There was a problem hiding this comment.
The try/except block that wraps the entire rfid_monitor code was not generated by AI 🙂. I intentionally like this approach because it allows any error to be propagated via error_queue; otherwise, the RFID process may terminate silently, or at least without reporting the exact error.
There was a problem hiding this comment.
I understand your logic, but nevertheless please restructure it (here and elsewhere) to focused catching of errors. try: except: statements should cover things that could go wrong through no fault of our software. For example, connecting to a serial port can go wrong if the device is not physically connected, so then we need to figure out what kind of error that gives and then catch exactly that error and only when it can occur. Right now, the try: except: statements cover a lot of code that simply should never crash unless there's a bug in our code, and that's not good style.
|
|
||
| except Exception as e: | ||
| stop_event.set() | ||
| error_queue.put(traceback.format_exc()) |
There was a problem hiding this comment.
🔴 This is a serious error! traceback is not imported, so this will crash. The more worrying aspect of this, is that it was clearly never tested, because it could never have worked.
There was a problem hiding this comment.
Done. (this had been tested in _rfid_lid650_665.py, but I forgot to add the import when porting the code to _rfid_base.py).
- Added the missing traceback import in omm_detect_participant - Remove some unused import or vars
|
It mostly looks good now and the test experiment also run. 👍 I'll wait for the |
…ad, socket send). The rest of the logic is now outside try/except
smathot
left a comment
There was a problem hiding this comment.
A few more small comments before I merge.
| return False | ||
|
|
||
|
|
||
| if is_raspberry_pi(): |
There was a problem hiding this comment.
This function is defined twice. Both incarnations have issues. The above shouldn't use a try except, because this code doesn't ever need to fail. The below doesn't check whether the current device is a raspberry bi, but rather whether a specific module is available. Also note that the try except should only include the actual import. Let's clean this up and clarify what it actually does.
There was a problem hiding this comment.
This function is not defined twice; it is defined only once and then used to perform the import of the GPIO module, which is only available on Raspberry Pi. I removed the try: block from the method implementation.
I am aware that this part of the code is not really portable to other users and is specific to my system. I think that, in the plugin, it would be better to add a graphical option such as: “Enable sound control using a Raspberry Pi GPIO pin”, followed by the pin number.
In any case, this part is intended to be changed in the future.
There was a problem hiding this comment.
Right, sorry, I misread it.
| print("Library RPi.GPIO not installed.") | ||
|
|
||
|
|
||
| PIN_RPY_SOUND_CTRL = 8 |
There was a problem hiding this comment.
Constants should be defined at the top below imports, and no non-standard empty lines. (Maybe a single one for clarity.)
| ) + 1 # 1 sec min, and 0.004s/step | ||
| time.sleep(motor_pause) | ||
| except Exception as e: | ||
| print(f"Error in _reward thread : {e}") |
There was a problem hiding this comment.
This comment applies to all print() statements except those in a separate process. Use oslogger.info/warning/error and not plain print statements.
| import serial | ||
|
|
||
| DEFAULT_PORT = 'COM4' | ||
| DEFAULT_PORT = "/dev/ttyConditioner" |
There was a problem hiding this comment.
This is a nonstandard port name. Is this what the device is called on your system?
There was a problem hiding this comment.
Yes, the port names are fixed this way using udev rules: /dev/ttyRFID and /dev/ttyConditionner on my system as on Nicolas system now.
This ensures that the ports always keep the same name and are not dependent on the order in which the devices are connected.
| self.start = kwargs.get("start", "S") # Default start signal | ||
| if not isinstance(self.start, str): | ||
| raise TypeError( | ||
| f"'start' doit être une chaîne, mais a reçu {type(self.start).__name__}" |
There was a problem hiding this comment.
Let's use only English to avoid a messy mix of languages.
- Minor formatting changes around try: blocks
|
Thanks for working with me on this guys 👍 |
Strasbourg modifications overview :
Rousset modifications overview :