-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor, Protocol Enhancements, and Device Ecosystem Reorganization #3
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
base: master
Are you sure you want to change the base?
Conversation
…mode Dev/editable mode
Added ReadS32 and refactored WriteS32 to handle array inputs
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
…end method Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
Co-authored-by: José Grilo <jose.grilo@research.fchampalimaud.org>
|
Hi! We made the following changes since the last meeting:
Notes
@glopesdev @bruno-f-cruz, feel free to give us feedback where improvements can be done. |
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.
Thanks for taking a stab at this. I left some first pass feedback.
I think we should discuss a bit the architecture of the device API synchronously during one of the meetings. I dont have a specific issue with it, but I am just afraid it may become hard to maintain if each register has its own specific read/write method with a name given without any sort of consistency.
- name: Clone harp.devices repository | ||
uses: actions/checkout@v4 | ||
with: | ||
repository: fchampalimaud/harp.devices |
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 am not sure the package repository should be coupled with the generation of docs for the devices.
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.
In the long term, we could consider to separate the docs generation from the packages in different repos. The current solution is not set in stone, but we would like to keep the documentation for the generic device and for the specific devices together.
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.
.vscode should be added to the list
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.
The reason we keep the .vscode
directory in the repo is because it has the extensions (and respective settings) we actively use for development and that we would like for contributors to use as well. Do you have an alternative suggestion regarding this matter?
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.
This should be changed to harp-tech, similar to what happens with the other software packages
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.
We can change it when the move of the repo to harp-tech
is imminent.
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.
Not sure what this config file is for? But should likely be ignored?
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.
This file was inherited from the AIND fork, but we agree it can be removed.
@@ -1,6 +1,7 @@ | |||
MIT License | |||
|
|||
Copyright (c) 2020 OEPS & Filipe Carvalho |
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.
We should consider making this standard with the copyright in the other software packages.
https://github.com/harp-tech/harp-python/blob/3d97d0b800533e4821896f744e52569c6413c6f7/LICENSE#L3
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.
Sure, same as answered previously,
We can change it when the move of the repo to harp-tech is imminent.
self, | ||
serial_port: str, | ||
dump_file_path: Optional[str] = None, | ||
read_timeout_s: float = 1, |
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.
Simplify the name to: timeout
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.
Agreed
_TODO_ | ||
""" |
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.
This is not up to date with the signature
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.
Thank you! We will update it.
# open file if it is defined | ||
if self._dump_file_path is not None: | ||
self._dump_file = open(self._dump_file_path, "ab") |
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.
Not sure I follow how you would want to use this.
I can see cases where users may want to skip querying the device for metadata, but this suggests that a valid use-case is to cache it?
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.
We have to look at this more carefully. We can discuss it further.
reply = self.send(HarpMessage.create(MessageType.READ, address, PayloadType.U8)) | ||
return OperationMode(reply.payload & OperationCtrl.OP_MODE) | ||
|
||
def dump_registers(self) -> list: |
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 think we should discuss a bit this API synchronously during one of the meetings. I dont dislike this but it has the potential to grow a bit out of control.
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.
This was only for this specific register where we don't know yet how to constrain the user's input regarding the OP_MODE to be of type OperationCtrl. In terms of usage, for this particular register, having the separated method made more sense. But we should discuss it further.
@@ -0,0 +1,22 @@ | |||
repos: |
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 would suggest turning this into an action instead of a pre-commit.
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.
Why an action only?
I might be missing something, but personally, I prefer to have checks locally before committing when possible.
We can discuss this further.
Important
We started working on this PR by merging the contributions made in PR#1
Refactoring & Project Structure
harp
and sub-namespace reorganization:harp.communication
for communication-related modulesharp.protocol
for core protocol definitionsharp.devices
for device-specific implementations (using PEP420 for implicit namespace packages)Features & Enhancements
Updated Harp protocol support with missing features
Updated currently known Harp devices names
Added context manager support to
Device
Added static factory method
HarpMessage.create
Changed dump file convention to be at the
Device
level instead of message levelRemoved
ReadXXXHarpMessage
andWriteXXXHarpMessage
classes andReadXXX
andWriteXXX
static methods fromHarpMessage
and moved them toDevice
Removed existing
behavior
driver because of newharp.behavior
namespace packageDocumentation
numpy
style convention for docstrings documentationdocs
folderDocumentation here: https://fchampalimaud.github.io/pyharp
Testing
Technical
uv
Contributors
harp.devices
harp.devices
namespace package for device-specific implementationsRepository here: https://github.com/fchampalimaud/harp.devices
As mentioned in issue #2, we were really interested in developing the Python Harp ecosystem further.
Our starting point was PR #1, which already had pushed
pyharp
forward (e.g., by implementing a way to deal with the device's events), and try to build on top of that. At first, we mainly focused on completing what was missing regarding the protocol (e.g., add a generic way to deal with the different payload types) and the interaction with a generic Harp device (e.g., add methods for the missing common registers).Our goal was to have a simple enough but versatile
pyharp
interface and we based our rationale to be as similar as possible as the Bonsai.Harp implementation. One way to achieve that was to get inspiration from theCreateMessage
Bonsai node to implement theHarpMessage.create
static method. Additionally, we added methods for theDevice
class that allow the users to read/write from/to registers that have different payload types.Another thing we tried to focus on was documentation. It surely can be improved but, at the moment, we documented every function and class of the project in order to facilitate the API usage and adapted/added some minimal examples that should help the user get started with the
pyharp
package.Beyond being possible to connect and interact with a generic Harp device, we also wanted to generate device-specific interfaces. On the one hand, we would like that each Harp device had its own package, so that if, for example, a user just wants to use
Harp Behavior
, it doesn't need to import a package that contains the interfaces for every board. This would also help when there's individual updates on each device. On the other hand, we would like that every package (includingpyharp
"core") had the same feeling, in the sense the user would get the implicit idea that every package is part of the same ecosystem. In order to achieve that, we organized the project directories according to PEP420, in which the global ecosystem has the following structure:We also wanted to have a way to generate the device-specific packages automatically, so that we could easily add new devices and keep the documentation up-to-date. For that, we used reflex-generator to generate the device-specific packages based on the device's YAML. Currently, the generated packages are available in the
harp.devices
repository mentioned above, but we believe that the Python interface should reside in each device's repository. PR#86 to reflex-generator has been submitted to support the Python package generation.Finally, we abandoned the
pyharp
name for the package because, unfortunately, someone registered a package in PyPI with the same name. The package name is currentlyharp-protocol
. At the same time, and mostly to keep coherence with the namespace naming convention used on C#/Bonsai side, we decided to change the global namespace toharp
.Known limitations of the PEP420 usage
To allow the usage of PEP420, we cannot have members under
harp
orharp.devices
that are not namespace packages. This means that we cannot have a__init__.py
file under those directories. As a consequence, we cannot have any code directly in those paths, which means that we cannot have any imports there.This also means that the
harp-python
package cannot currently be installed in the same venv until some changes happen there. Our suggestion at the time was to place it's contents underharp.data
and then import it from there. This would allow theharp-python
package to be installed in the same venv asharp-protocol
and the individual harp devices. We also submitted a PR to theharp-python
repository to reflect this change in PR#52.With these new PRs, we aim to expand the current features of the Python implementation and make it more accessible for new users, helping them adopt and engage with the HARP ecosystem. We also see value in consolidating the Python implementation within the official harp-tech repository, and we’re more than happy to help maintain it. This would help avoid fragmented development efforts and lead to a more unified and robust solution for the community. We will continue iterating on these developments and commit new features to this PR while we work toward a consensus on merging it into the harp-tech repository.