WHOIGit / PhytO-ARM

PhytO-ARM: Phytoplankton Observing for Automated Real-time Management
https://hablab.whoi.edu/phyto-arm/
GNU General Public License v3.0
0 stars 0 forks source link

Major refactor for coordinated multi-winch and testing #34

Closed figuernd closed 4 months ago

figuernd commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

rgov commented 4 months ago

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