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

feat: Implements JVL command for zeroing position encoder, calls on winch startup #22

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

figuernd
Copy link
Contributor

@figuernd figuernd commented Jun 2, 2023

No description provided.

@figuernd figuernd requested a review from rgov June 2, 2023 20:44
@rgov
Copy link
Member

rgov commented Jun 2, 2023

There's a typo in the service definition, "registry".

If this is the correct way to do it, then it seems like a good change.

@figuernd figuernd force-pushed the nathan.figueroa/zero-position-encoder branch from ec178fa to 2d0db0d Compare June 2, 2023 20:55
@figuernd
Copy link
Contributor Author

figuernd commented Jun 2, 2023

There's a typo in the service definition, "registry".

If this is the correct way to do it, then it seems like a good change.

Good catch, fixed

@figuernd
Copy link
Contributor Author

@rgov can you think of any side effects of using this? i.e. position drift from being reset too many times? (Seems unlikely since position is calibrated by CTD depth). Thinking about adding a config option to disable this command as a backup.

@rgov
Copy link
Member

rgov commented Jun 21, 2023

I have no experience outside this project. I don't think we need to add an option; either it works and we continue using it or it doesn't and we deploy a hot fix to revert back.

It would give me more confidence to know that this is indeed exactly how MacTalk resets the encoder position. We can do that by using MacTalk with WireShark, for example.

@figuernd figuernd force-pushed the nathan.figueroa/zero-position-encoder branch from 2d0db0d to 68d1ea7 Compare August 9, 2023 18:59
@figuernd
Copy link
Contributor Author

figuernd commented Sep 5, 2023

I have no experience outside this project. I don't think we need to add an option; either it works and we continue using it or it doesn't and we deploy a hot fix to revert back.

It would give me more confidence to know that this is indeed exactly how MacTalk resets the encoder position. We can do that by using MacTalk with WireShark, for example.

Haven't merged yet because I still hope to do this; haven't had the proper setup yet.

@figuernd figuernd force-pushed the nathan.figueroa/zero-position-encoder branch from 68d1ea7 to a21266a Compare March 13, 2024 18:19
@figuernd figuernd force-pushed the nathan.figueroa/zero-position-encoder branch from a21266a to ed3dd1b Compare July 22, 2024 20:28
@rgov
Copy link
Member

rgov commented Jul 22, 2024

I am not 100% sure about zeroing at startup since in theory the zero point would best be the median depth? If the total encoder range is more than enough to cover the depth range that's probably fine though.

Merge when you're happy with it!

@figuernd
Copy link
Contributor Author

figuernd commented Aug 7, 2024

I'm going to take a closer look at MacTalk values in the lab next week - but we've been using this in the field long enough that I'm going to merge for now so that I can finally stop rebasing on top of it for every deployment. Will open a new issue if I find anything wrong with the procedure in lab.

@figuernd figuernd closed this Aug 7, 2024
@figuernd figuernd reopened this Aug 7, 2024
@figuernd figuernd merged commit 32973ac into main Aug 7, 2024
2 of 3 checks passed
@figuernd figuernd deleted the nathan.figueroa/zero-position-encoder branch December 23, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants