Skip to content
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

Over The Air device config feature #48

Merged
merged 29 commits into from
Nov 6, 2024
Merged

Over The Air device config feature #48

merged 29 commits into from
Nov 6, 2024

Conversation

t-sasatani
Copy link
Collaborator

@t-sasatani t-sasatani commented Oct 24, 2024

This PR adds modules and CLI to update device parameters over the air.

To-dos

Add more commands

  • Gain
  • FOV

Tests

  • Command generation
  • Models and validation
  • Mock device I/O (though not sure what's the best way because this is one-way communication)

Others/low priority

  • Documentation
  • Integrate with the capture command(Future work)
  • Check metadata and validate device update (plotting and checking by eye might be good for now)(Future work)

@coveralls
Copy link
Collaborator

coveralls commented Oct 24, 2024

Coverage Status

coverage: 79.23% (+4.6%) from 74.586%
when pulling 7e0a836 on feature_ir_update
into 0d1b992 on main.

@sneakers-the-rat
Copy link
Collaborator

sneakers-the-rat commented Oct 24, 2024

hell ya. lmk when you'd like a review :)

@t-sasatani
Copy link
Collaborator Author

Yup will ping you in a bit!

@t-sasatani t-sasatani changed the title Remote device update feature Over The Air device update feature Oct 27, 2024
@t-sasatani t-sasatani added the enhancement New feature or request label Oct 27, 2024
@t-sasatani t-sasatani changed the title Over The Air device update feature Over The Air device config feature Oct 27, 2024
@t-sasatani t-sasatani marked this pull request as ready for review November 1, 2024 10:35
@t-sasatani
Copy link
Collaborator Author

@sneakers-the-rat I'd appreciate it if you could have a look. I'm mostly unsure what a good way to test device I/O is.

@t-sasatani
Copy link
Collaborator Author

t-sasatani commented Nov 2, 2024

Added some tests that cover device I/Os (might be a bit too manual) and docs for the hardware. Docs are very minimal but should be good for now.

Copy link
Collaborator

@sneakers-the-rat sneakers-the-rat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions on error messages, but overall i'd be fine with merging this as long as we raise an issue to remind us to write a full mock of the Serial device later. I would really prefer we stick to python naming conventions and use snake case for function names, but don't want to be a dictator about it.

Other than that, hopefully comments are self explanatory

miniscope_io/cli/update.py Outdated Show resolved Hide resolved
miniscope_io/cli/update.py Outdated Show resolved Hide resolved
miniscope_io/cli/update.py Outdated Show resolved Hide resolved
miniscope_io/cli/update.py Show resolved Hide resolved
miniscope_io/cli/update.py Outdated Show resolved Hide resolved
miniscope_io/models/devupdate.py Outdated Show resolved Hide resolved
miniscope_io/models/devupdate.py Show resolved Hide resolved
miniscope_io/models/devupdate.py Outdated Show resolved Hide resolved
miniscope_io/models/devupdate.py Outdated Show resolved Hide resolved
tests/test_device_update.py Outdated Show resolved Hide resolved
@sneakers-the-rat
Copy link
Collaborator

sneakers-the-rat commented Nov 6, 2024

oh crap sorry i hit rebase and forgot that it adds me as co-author on all the commits :'\. should have waited for u

@t-sasatani
Copy link
Collaborator Author

Thanks for the comments! I'll look into it in a bit. And the commits are no problem; I'm not sure if I can interact with origin this week anyway.

t-sasatani and others added 3 commits November 6, 2024 14:20
Co-authored-by: Jonny Saunders <JLSaunders987@gmail.com>
Co-authored-by: Jonny Saunders <JLSaunders987@gmail.com>
Co-authored-by: Jonny Saunders <JLSaunders987@gmail.com>
@@ -74,7 +74,9 @@ def validate_values(cls, values: dict) -> dict:
elif target in UpdateTarget:
raise NotImplementedError()
else:
raise ValueError(f"{target} is not a valid update target, need an instance of UpdateTarget")
raise ValueError(
f"{target} is not a valid update target," "need an instance of UpdateTarget"
Copy link
Collaborator

@sneakers-the-rat sneakers-the-rat Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this happens to me literally all the time where i want to split a line for aesthetic reasons and then black is like "no you should not have split that line." it borders on malicious compliance and i imagine black having a smile on its face every time it does this like "you were the one who set the line width!"

Copy link
Collaborator Author

@t-sasatani t-sasatani Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was fully trusting black and wasn't even paying attention to the changes it made lol. Didn't know it was making my code ugly!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it is literally equivalent to the interpreter after parsing (?), but at the same time there is something bad about ("start " "end") vs "start end" that feels like the machines are taking revenge when they make it happen.

@t-sasatani t-sasatani merged commit fa69f0d into main Nov 6, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants