Skip to content

Select hardware based on config.json file#602

Merged
ethanjli merged 20 commits intomasterfrom
refactor/unify-device-controller-service
May 20, 2025
Merged

Select hardware based on config.json file#602
ethanjli merged 20 commits intomasterfrom
refactor/unify-device-controller-service

Conversation

@ethanjli
Copy link
Copy Markdown
Collaborator

@ethanjli ethanjli commented May 8, 2025

As part of #182, this PR enables the user to switch between the adafruithat vs. planktoscopehat program variants by editing the SD card's ~/PlanktoScope/config.json file and then rebooting (or restarting nodered.service and then restarting planktoscope-org.device-backend.controller.service after Node-RED becomes ready to receive MQTT messages, without a full system reboot). A minor caveat here is that GPS functionality is not available on planktoscopehat-based images running the adafruithat program variants (because of #591); but in the 2025-05-14 software meeting I plan to propose deprecating GPS functionality from the adafruithat variants too.

Specifically, this PR:

  • unifies the systemd services for the Python device controller into a single service, and this PR provides a single top-level entrypoint script which selects the adafruithat vs. planktoscopehat main.py script based on the configuration in /usr/share/planktoscope/installer-config.yml, in the same way that software: Gitify node-red-dashboard #573 selects adafruithat vs. planktoscopehat for the Node-RED dashboard ~/PlanktoScope/config.json file's acq_instrument field (whose values are like PlanktoScope v2.6). See Use runtime to select the hardware variant #583 (comment) and the 2025-05-07 software meeting for additional context on this step towards a piecemeal incremental unification of the adafruithat vs. planktoscopehat variants of the Python device controller.
  • updates the Node-RED dashboard's settings.js file to choose between the adafruithat vs. planktoscopehat variants of the dashboard based on the ~/PlanktoScope/config.json file's acq_instrument field.
  • fixes a regression in the fairscope-latest image caused by software: Gitify node-red-dashboard #573, where the fairscope-latest image was unable to select the planktoscopehat Node-RED dashboard (because it was looking for a fairscope-latest variant of the dashboard).

This PR also removes OS setup script support for "segmenter-only" images, since those images were never officially supported (and never documented in https://docs.planktoscope.community/reference/software/subsystems/installation/) and we stopped building them with #573.

TODOs:

  • Move the if __name__ == "__main__" code blocks in the adafruithat/planktoscopehat main.py files into a main function, so that we can invoke those scripts more cleanly, and so that we can make loguru-based logging work properly. This task is blocked by backend: Read hardware config once #600 due to the risk of messy merge conflicts with that PR.
  • Move the loguru-based logger from the adafruithat/planktoscopehat main.py files to the hardware controller's top-level main.py file, so that log messages will be saved to file.
  • Ensure that uncaught exceptions in the code are are logged to file by a top-level exception catcher via loguru.

For future PRs (since this PR already touches a lot of files):

  • Move the .../control/planktoscopehat/planktoscope/identity.py & .../control/planktoscopehat/planktoscope/mqtt.py & .../control/planktoscopehat/planktoscope/camera & .../control/planktoscopehat/planktoscope/imaging driver modules to a shared, unified location (perhaps ../control/drivers) which is used by both .../planktoscopehat/main.py and .../adafruithat/main.py, instead of having two (supposedly) identical of each module.
  • Move everything in .../control/{variant}/planktoscope to .../control/{variant}, to reduce redundancies the path names

@ethanjli ethanjli changed the title Unify hardware controller variants into one entrypoint script Select adafruithat vs. planktoscopehat program variants based on config.json file May 8, 2025
@ethanjli
Copy link
Copy Markdown
Collaborator Author

ethanjli commented May 8, 2025

@sonnyp This PR's design choices may have some minor conflicts with certain design choices you made in #583. Here are some potentially-relevant assumptions I've made for this PR - they may be worth revisiting in subsequent work in #583:

  • If ~/PlanktoScope/config.json is missing, we'll log a warning and then default to selecting the planktoscopehat variant of both the Node-RED dashboard (which displays the version-selection menu on the home page when config.json's acq_instrument field has value null) and the hardware controller. I log a warning because, until you merge Use runtime to select the hardware variant #583, I believe a nonexistent config.json is an exceptional (and potentially unhandled) situation.

  • If .../config.json has a null value for acq_instrument, that is the expected initial value to indicate that we should select the planktoscopehat variant of the programs. This matches the current contents of ~/PlanktoScope/software/node-red-dashboard/default-configs/planktoscopehat-latest.config.json.

  • It's fine to run a particular variant (e.g. planktoscopehat) of the hardware controller by default. I made this decision so that I could make the variant-selection code basically line-for-line equivalent between ~/PlanktoScope/device-backend/control/main.py and ~/PlanktoScope/software/node-red-dashboard/settings.js, for maintainability reasons.

    If we don't like this assumption: then we can just make .../control/main.py quit early (with either a zero or non-zero return code) if .../config.json is missing or if .../config.json's acq_instrument field is null, instead of launching .../control/planktoscopehat/main.py.

  • It's fine to run a particular variant (e.g. planktoscopehat) of the Node-RED dashboard by default.

    If we don't like this assumption: then the version-selection dialogue will need to be implemented outside of Node-RED (e.g. in your rewrite of device-portal).

If you want to merge #583 by having that PR rely on the planktoscopehat Node-RED dashboard's home page's version-selection menu for selecting the hardware variant, then I believe you'll need to make one of the following sets of changes to the planktoscopehat Node-RED dashboard in #583:

  1. Restart nodered.service before restarting planktoscope-org.device-backend.controller.service (so that when the hardware controller starts it will announce the camera name/ID to the Node-RED dashboard, because Add MQTT Handling for Retrieving and Publishing Camera Name PlanktoScope/device-backend#44 was never merged) after a version is selected from the home page.
  2. Trigger a full system reboot after a version is selected from the home page (and after config.json and hardware.json are correctly populated), to simplify the implementation of correct functionality in the Node-RED dashboard.

Copy link
Copy Markdown
Collaborator

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Please undo formatting changes to facilitate review

Comment thread device-backend/control/main.py Outdated
@sonnyp sonnyp changed the title Select adafruithat vs. planktoscopehat program variants based on config.json file Select hardware based on config.json file May 20, 2025
@sonnyp
Copy link
Copy Markdown
Collaborator

sonnyp commented May 20, 2025

I took the liberty of finishing this.

Regarding

Ensure that uncaught exceptions in the code are are logged to file by a top-level exception catcher via loguru.

This works as expected:

image

@sonnyp
Copy link
Copy Markdown
Collaborator

sonnyp commented May 20, 2025

Regarding

For future PRs (since this PR already touches a lot of files):

I'm not convinced. I'd much prefer the 2 codebases to remain separate unless someone has a plan on doing all the work for the merge.

Otherwise, planktoscopehat will get in the way of adafruithat and adafruithat will get in the way of planktoscopehat without a clear benefit.

@sonnyp sonnyp marked this pull request as ready for review May 20, 2025 17:23
@sonnyp
Copy link
Copy Markdown
Collaborator

sonnyp commented May 20, 2025

You can review my changes with

git checkout refactor/unify-device-controller-service
git pull
git diff 4103b3b4f4a07249c9239a84ddc996421cb96d18

@ethanjli
Copy link
Copy Markdown
Collaborator Author

Sorry about the delay on my end / thanks for finishing this! Your changes look good to me, so I'll press the "Squash and merge" button. I'll follow up in another comment reply regarding the future-PR TODOs I had proposed.

@ethanjli ethanjli merged commit 5bf4c89 into master May 20, 2025
5 checks passed
@ethanjli ethanjli deleted the refactor/unify-device-controller-service branch May 20, 2025 17:50
@ethanjli
Copy link
Copy Markdown
Collaborator Author

ethanjli commented May 20, 2025

I'm not convinced. I'd much prefer the 2 codebases to remain separate unless someone has a plan on doing all the work for the merge.

Otherwise, planktoscopehat will get in the way of adafruithat and adafruithat will get in the way of planktoscopehat without a clear benefit.

Having two identical copies of variant-independent code (i.e. the camera/imaging subsystem, generic-ish MQTT adapter, and machine-name support modules) makes their source code prone to drifting from each other unless we maintain a discipline of replicating changes to those modules from one variant's codebase to the other variant's codebase (e.g. when I reviewed #600, I totally missed the fact that you changed code files for the camera/imaging subsystem only for the planktoscopehat variant and not for the adafruithat variant). This reasoning is also part of why I had split the segmenter's codebase from the hardware controller's codebase, because it makes no sense for the segmenter to behave differently between adafruithat vs. planktoscopehat. In those cases (i.e. for the segmenter, the camera/imaging subsystem, the generic-ish MQTT adapter, and machine-name support), I think variant drift should be treated as maintainer error, because it increases the complexity of mental models needed for helping users troubleshoot their problems. To me, the benefit of having a single copy of those variant-independent modules is that it removes the root cause of such variant drift.

With that said, I'm not opposed to leaving two copies of those variant-independent modules if we have some other way of mitigating the problems caused by that. For example, how would you feel about a potential alternative where I just implement a poetry run poe check step (which thus would be part of our CI checks) which fails if the code files for those specific modules (camera/imaging subsystem, generic-ish MQTT adapter, and machine-name support) are different between the two variants?

@sonnyp
Copy link
Copy Markdown
Collaborator

sonnyp commented May 21, 2025

If adafruithat is in maintenance mode as we discussed then code drifting is a feature not a bug in my view.

  • Contributors should be able to make changes to the codebase without having to worry or test adafruithat (which they might not be able to do)
  • We should minimize risks of breakage as much as possible and as such avoid changes to adafruithat

If we want to keep supporting adafruithat as a fully supported platform then yes I agree code paths should be merged but fully, not "halfway" or we will get the worst of both worlds.

Let's discuss this at the weekly meeting today?

@sonnyp
Copy link
Copy Markdown
Collaborator

sonnyp commented May 21, 2025

There is a regression which I don't understand

starting or restarting planktoscope-org.device-backend.controller.service takes about 45s

$ time sudo systemctl start planktoscope-org.device-backend.controller.service 

real    0m43.383s
user    0m0.007s
sys     0m0.000s

Any idea @ethanjli ?

@ethanjli
Copy link
Copy Markdown
Collaborator Author

ethanjli commented May 21, 2025

Yesterday I also noticed it during shutdown. Maybe we'll also need to lift interrupt signal handling to the top level, or something? That doesn't explain the slow startup times, though.

@sonnyp
Copy link
Copy Markdown
Collaborator

sonnyp commented May 21, 2025

No logs before 45 seconds either in journalctl or ~/device-backend-logs

@ethanjli
Copy link
Copy Markdown
Collaborator Author

ethanjli commented May 21, 2025

Let's discuss this at the weekly meeting today?

For anyone looking at this PR in the future, we've resolved this discussion in the 2025-05-21 core team meeting, and the decision is recorded there

sonnyp added a commit that referenced this pull request Jun 19, 2025
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