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

Major refactor for coordinated multi-winch and testing #34

Merged
merged 39 commits into from
May 16, 2024

Conversation

figuernd
Copy link
Contributor

@figuernd figuernd commented Feb 29, 2024

This represents a major overhaul which adds a number of new features, including:

  1. Support for multiple winches coordinated by a central "lock_manager".
  2. Simplified mission programming with an "arm" oriented event-driven architecture.
  3. Consolidation of winched and winchless code.
  4. Discrete launching for the main nodes and arm nodes. This allows individual arms and their winches to fail or be relaunched with new parameters without disrupting other arm nodes at work.
  5. Unit tests for the phyto_arm package. Code coverage is still incomplete but a pattern for testing has been established and ~35 tests written.
  6. Development container support, enabling development in an identical environment no matter the development host.
  7. Hardware mocking, allowing PhytO-ARM to be run entirely on developer workstations with no need for a development IFCB, motors or CTDs.
  8. Config validation for ensuring PhytO-ARM is launched with a config file that meets a simple spec.
  9. Multi-camera support.
  10. Convenience scripts for ease of starting and stopping in Docker and tmux.

Squashed commit history:

feat: Node and tests for validating configuration

feat: Added launch configuration

fix: config validation nodes executable, validate seatrac

fix: missing shebang

feat: Moved validation from nodes to start script

refactor: Multiple winches WIP. New conductor

refactor: Winch movement

refactor: Instrument actions

feat: Fixed service message locations

feat: Added arm controller for RBR

feat: Node for publishing UDP stream, corrected CTD paths

build: dev container configuration

feat: Multi-camera support

fix: config and build fixes found during testing

feat: Better buffering for RBR stream, service definition for running relay on another device

fix: service import and usage errors

debug: runtime progress

fix: Improved namespace handling, debug logging

fix: Moved instrument specific config to respective arms

fix: Include depth from RBR readings

feat: Misc fixes to paths and parameter passing

feat: Improves task behavior, eliminates race condition

refactor: Use semaphores and events instead of locks

feat: Will run scheduled depth when profile fails to gather enough data

feat: Nomenclature update: Instrument->Payload

feat: Convert state class to dataclass

refactor: reorganized task parameters in yaml

refactor: Move winch control to arm base class

feat: Move IFCB operation out of ARM for easier mocking

feat: Hardware mocking for local development/testing

feat: Upgrade aiohttp

fix: IFCB action call error, improvements to naming, loop locking

feat: Switch from queue to event-driven tasks. Add wiz_probe behavior

test: Unit tests for scheduled depth and wiz behaviors

feat: per-task winch speed, chanos arm using RBR sensor

feat: Removed files from old multiwinch method

fix: Removed unused srv folder/reference

fix: Ensure required version of setuptools is available

feat: Removed unused files/references

feat: Config validation skip switch for launch

feat: Moved mock nodes into their own directory

fix: Misc typos

fix: RBR base behavior fixes

refactor: Arm nodes are now launched independently of core PhytO-ARM nodes

refactor: phyto-arm start now takes a launchfile name arg

fix: Check correct semaphore signal

docs: Comments

doc: Updated documentation

feat: renamed arm launchfiles for consistency

feat: Adjusted chanos config for specific depths

feat: Better exceptions for out of bounds cases in arm base

feat: Added support for wiz probe depth offset config

docs: Better example depths

feat: Switched to named locking mechanism to allow crashed arms to reenter existing locks

feat: Convenience running scripts, fixes missing scripts from build

fix: Misc run script issues

fix: docker image ref

feat: Added development section to README

feat: A little more tolerance for malformed RBR messages

feat: Cleaned up example.yaml

@figuernd figuernd requested review from rgov and joefutrelle February 29, 2024 20:12
Copy link
Member

@rgov rgov left a comment

Choose a reason for hiding this comment

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

This is a partial review. Some simple changes have been put on the rzg/multi-winch-review branch.

I sometimes go a little overboard with rebasing but it is a great way to prepare branches for pull requests by splitting and combining commits into logical changes. Then it would be easier to review different parts of this. It seems like the logical commits would be something like:

  1. Add support for dev containers
  2. Add unit testing
  3. Add support for the Wiz
  4. Add support for multiple winches
  5. ...

It seems like there were multiple commits that got merged together into one über-commit. Can we get those back?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
scripts/config_validation.py Show resolved Hide resolved
src/phyto_arm/src/arm_ifcb.py Outdated Show resolved Hide resolved
src/phyto_arm/src/arm_ifcb.py Outdated Show resolved Hide resolved
src/phyto_arm/src/arm_ifcb.py Outdated Show resolved Hide resolved
src/phyto_arm/src/arm_base.py Outdated Show resolved Hide resolved
@figuernd
Copy link
Contributor Author

figuernd commented Mar 1, 2024

This is a partial review. Some simple changes have been put on the rzg/multi-winch-review branch.

I sometimes go a little overboard with rebasing but it is a great way to prepare branches for pull requests by splitting and combining commits into logical changes. Then it would be easier to review different parts of this. It seems like the logical commits would be something like:

1. Add support for dev containers

2. Add unit testing

3. Add support for the Wiz

4. Add support for multiple winches

5. ...

It seems like there were multiple commits that got merged together into one über-commit. Can we get those back?

Yeah I thought it would be easier to review the finished product because some detours were taken in the full commit history, but you can see the pre-squashed commits here: https://github.com/WHOIGit/PhytO-ARM/tree/nathan.figueroa/multi-winch-backup

@figuernd figuernd force-pushed the nathan.figueroa/multi-winch branch 2 times, most recently from 19863c1 to ec71501 Compare March 1, 2024 22:55
@figuernd
Copy link
Contributor Author

I've gone through with pylint and applied most suggestions. I do have the following configurations applied however:

--disable=missing-function-docstring, missing-class-docstring, missing-module-docstring, global-statement, unused-argument, import-error, invalid-name, unnecessary-lambda-assignment
--max-line-length=120

I've also ensured a blank line before most comments, with the exception of inline comments and comments immediately following a function heading or if statement.

@figuernd figuernd force-pushed the nathan.figueroa/multi-winch branch from 86d2f9c to 37e645b Compare March 12, 2024 20:17
rgov
rgov previously requested changes Mar 18, 2024
phyto-arm Show resolved Hide resolved
@figuernd figuernd force-pushed the nathan.figueroa/multi-winch branch 2 times, most recently from 4864344 to c180df0 Compare May 14, 2024 19:17
@figuernd figuernd requested a review from rgov May 14, 2024 20:03
@rgov
Copy link
Member

rgov commented May 14, 2024

I'd say rebase and merge if this is tested.

@figuernd figuernd force-pushed the nathan.figueroa/multi-winch branch from 07bd929 to 1f2656b Compare May 15, 2024 21:44
figuernd and others added 14 commits May 15, 2024 23:06
…king, unit tests, config validation, and dev containers.

feat: Node and tests for validating configuration

feat: Added launch configuration

fix: config validation nodes executable, validate seatrac

fix: missing shebang

feat: Moved validation from nodes to start script

refactor: Multiple winches WIP. New conductor

refactor: Winch movement

refactor: Instrument actions

feat: Fixed service message locations

feat: Added arm controller for RBR

feat: Node for publishing UDP stream, corrected CTD paths

build: dev container configuration

feat: Multi-camera support

fix: config and build fixes found during testing

feat: Better buffering for RBR stream, service definition for running relay on another device

fix: service import and usage errors

debug: runtime progress

fix: Improved namespace handling, debug logging

fix: Moved instrument specific config to respective arms

fix: Include depth from RBR readings

feat: Misc fixes to paths and parameter passing

feat: Improves task behavior, eliminates race condition

refactor: Use semaphores and events instead of locks

feat: Will run scheduled depth when profile fails to gather enough data

feat: Nomenclature update: Instrument->Payload

feat: Convert state class to dataclass

refactor: reorganized task parameters in yaml

refactor: Move winch control to arm base class

feat: Move IFCB operation out of ARM for easier mocking

feat: Hardware mocking for local development/testing

feat: Upgrade aiohttp

fix: IFCB action call error, improvements to naming, loop locking

feat: Switch from queue to event-driven tasks. Add wiz_probe behavior

test: Unit tests for scheduled depth and wiz behaviors

feat: per-task winch speed, chanos arm using RBR sensor

feat: Removed files from old multiwinch method

fix: Removed unused srv folder/reference

fix: Ensure required version of setuptools is available

feat: Removed unused files/references

feat: Config validation skip switch for launch

feat: Moved mock nodes into their own directory

fix: Misc typos

fix: RBR base behavior fixes

refactor: Arm nodes are now launched independently of core PhytO-ARM nodes

refactor: phyto-arm start now takes a launchfile name arg

fix: Check correct semaphore signal

docs: Comments

doc: Updated documentation

feat: renamed arm launchfiles for consistency

feat: Adjusted chanos config for specific depths

feat: Better exceptions for out of bounds cases in arm base

feat: Added support for wiz probe depth offset config

docs: Better example depths

feat: Switched to named locking mechanism to allow crashed arms to reenter existing locks

feat: Convenience running scripts, fixes missing scripts from build

fix: Misc run script issues

fix: docker image ref

feat: Added development section to README

feat: A little more tolerance for malformed RBR messages

feat: Cleaned up example.yaml
figuernd and others added 25 commits May 15, 2024 23:06
Updated hard coded header count from 16 to 15 for change in RBR unit used in Alma March 2023 deployment.
Lat and Lon need float values in example.yaml for proper config validation.
Changed with alternate camera names, uncommented 2nd ip cam.
Changed camera subnames for consistency with changes in main.launch file
Revert back to fore/aft camera designations. "aft_camera" uncommented.
Revert back to fore/aft camera designations.
@figuernd figuernd force-pushed the nathan.figueroa/multi-winch branch from 1f2656b to 07e72ff Compare May 16, 2024 03:08
@figuernd figuernd dismissed rgov’s stale review May 16, 2024 15:45

I've addressed all conversations but there's one I cannot reach anymore because of a commit hash change and therefore cannot mark resolved.

@figuernd figuernd merged commit e760383 into main May 16, 2024
2 checks passed
@figuernd figuernd deleted the nathan.figueroa/multi-winch branch May 16, 2024 15:46
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.

3 participants