From 0b00ac266182e2bbf303824fc75c47be15b2cf7b Mon Sep 17 00:00:00 2001 From: David Hale Date: Tue, 13 Oct 2026 11:55:27 -0700 Subject: [PATCH 1/2] server hands any name it doesn't dispatch to controller_cmd mode is made a core function. is_instrument_command is removed --- .github/workflows/emulator-integration.yml | 6 ++ README.md | 2 +- camerad/archon_interface.cpp | 23 +++---- camerad/archon_interface.h | 8 ++- camerad/camera_interface.h | 17 +++-- camerad/camera_server.cpp | 69 +++---------------- docs/_ext/camerad_tables.py | 27 ++++++-- docs/architecture/index.md | 8 ++- docs/commands/base.md | 16 +++-- docs/commands/controller.md | 10 +-- docs/data/commands.yaml | 15 ++-- docs/python/index.md | 8 +-- python/camera_interface_module.cpp | 20 +++--- python/examples/raw_channel_sweep.py | 2 +- python/tests/emulator_integration.py | 9 +-- reference_instrument/README.md | 3 - reference_instrument/reference_instrument.cpp | 22 +----- reference_instrument/reference_instrument.h | 1 - 18 files changed, 107 insertions(+), 159 deletions(-) diff --git a/.github/workflows/emulator-integration.yml b/.github/workflows/emulator-integration.yml index f2ace2d..a4e9af0 100644 --- a/.github/workflows/emulator-integration.yml +++ b/.github/workflows/emulator-integration.yml @@ -166,5 +166,11 @@ jobs: expect "exposuremode SINGLE" "^SINGLE DONE" expect "expose 1" "DONE" + # mode is a core function; getp is the Archon interface's own command, + # reached through controller_cmd; a name nobody claims is an error + expect "mode DEFAULT" "^DONE" + expect "getp nRowsQuad" "^[0-9]" + expect "nosuchcommand" "^not_supported ERROR" + - name: Reference module import test run: ctest --test-dir build-reference --output-on-failure diff --git a/README.md b/README.md index 78122c9..674949a 100644 --- a/README.md +++ b/README.md @@ -261,7 +261,7 @@ camera.instrument_cmd("roi", "51 60 51 60") print(camera.output_status()) # frames written, dropped, last file ``` -Every command `camerad` accepts is reachable: the base commands are bound as methods, instrument-specific ones go through `instrument_cmd()` (enumerable with `instrument_commands()`), and controller-specific ones such as `mode`, `raw`, `readacf`, `loadtiming`, `heater` and `sensor` go through `controller_cmd()`. Only `exit` is omitted, since the process belongs to the caller. +Every command `camerad` accepts is reachable: the base commands, `mode` among them, are bound as methods, instrument-specific ones go through `instrument_cmd()` (enumerable with `instrument_commands()`), and controller-specific ones such as `raw`, `readacf`, `loadtiming`, `heater` and `sensor` go through `controller_cmd()`, which forwards any other name to the instrument as `camerad` does. Only `exit` is omitted, since the process belongs to the caller. The controller and instrument are fixed at CMake configure time, so `instrument_name()` and `controller_name()` report which build was loaded. A failed command raises `RuntimeError`. diff --git a/camerad/archon_interface.cpp b/camerad/archon_interface.cpp index ea4d818..7c0a9db 100644 --- a/camerad/archon_interface.cpp +++ b/camerad/archon_interface.cpp @@ -69,10 +69,9 @@ namespace Camera { /***** Camera::ArchonInterface::controller_cmd ******************************/ /** * @brief dispatcher for Archon-specific commands - * @details This allows dispatching Archon controller specific commands by - * receiving the commands and args and calling the appropriate - * controller-specific function. This should be calling - * ArchonInterface functions. + * @details The server calls this for every name the core does not + * recognize centrally. It handles the commands unique to the + * Archon and passes any other name to instrument_cmd. * @param[in] cmd command * @param[in] args argument list * @param[out] retstring return string @@ -102,10 +101,6 @@ namespace Camera { return this->set_parameter(args, retstring); } else - if ( cmd == CAMERAD_MODE ) { - return this->set_camera_mode(args, retstring); - } - else if ( cmd == CAMERAD_RAW ) { return this->raw(args, retstring); } @@ -122,8 +117,7 @@ namespace Camera { return this->sensor(args, retstring); } else { - retstring="unrecognized command"; - return ERROR; + return this->instrument_cmd(cmd, args, retstring); } } /***** Camera::ArchonInterface::controller_cmd ******************************/ @@ -802,7 +796,7 @@ namespace Camera { /***** Camera::ArchonInterface::load_timing *********************************/ - /***** Camera::ArchonInterface::set_camera_mode *****************************/ + /***** Camera::ArchonInterface::camera_mode *********************************/ /** * @brief set a camera "mode" * @details Allowable camera modes are defined in the ACF file and specify @@ -816,8 +810,8 @@ namespace Camera { * @return ERROR|NO_ERROR * */ - long ArchonInterface::set_camera_mode(std::string args, std::string &retstring) { - const std::string function("Camera::ArchonInterface::set_camera_mode"); + long ArchonInterface::camera_mode( const std::string args, std::string &retstring ) { + const std::string function("Camera::ArchonInterface::camera_mode"); // Help if (args=="?" || args=="help") { retstring = CAMERAD_MODE; @@ -833,6 +827,9 @@ namespace Camera { // call the work function return( this->set_camera_mode(args) ); } + /***** Camera::ArchonInterface::camera_mode *********************************/ + + /***** Camera::ArchonInterface::set_camera_mode *****************************/ /** * @brief set a camera "mode" diff --git a/camerad/archon_interface.h b/camerad/archon_interface.h index dff5231..56126a6 100644 --- a/camerad/archon_interface.h +++ b/camerad/archon_interface.h @@ -32,6 +32,7 @@ namespace Camera { long basename( const std::string args, std::string &retstring ) override; long bias( const std::string args, std::string &retstring ) override; long bin( const std::string args, std::string &retstring ) override; + long camera_mode( const std::string args, std::string &retstring ) override; long connect_controller( const std::string args, std::string &retstring ) override; long disconnect_controller( const std::string args, std::string &retstring ) override; long exptime( const std::string args, std::string &retstring ) override; @@ -48,11 +49,13 @@ namespace Camera { long do_expose() override; - // Archon controller command dispatcher + // Archon controller command dispatcher. The server calls it for every + // name the core does not recognize centrally; it handles the Archon + // commands and passes any other name to instrument_cmd // long controller_cmd(const std::string &cmd, const std::string &args, - std::string &retstring) override; + std::string &retstring) final; // These functions are specific to the Archon Interface and are not // found in the base class. @@ -64,7 +67,6 @@ namespace Camera { long load_timing(std::string cmd, std::string &reply); long read_acf(const std::string &filename); long set_parameter(const std::string &args, std::string &retstring); - long set_camera_mode(std::string args, std::string &retstring); long set_camera_mode(std::string modeselect); long set_vcpu_inreg(const std::string &args, std::string &retstring); long heater(const std::string &args, std::string &retstring); diff --git a/camerad/camera_interface.h b/camerad/camera_interface.h index 95d7d86..d5dba78 100644 --- a/camerad/camera_interface.h +++ b/camerad/camera_interface.h @@ -112,6 +112,7 @@ namespace Camera { virtual long basename( std::string args, std::string &retstring ) = 0; virtual long bias( std::string args, std::string &retstring ) = 0; virtual long bin( std::string args, std::string &retstring ) = 0; + virtual long camera_mode( std::string args, std::string &retstring ) = 0; ///< selects a mode, camerad's named operational configuration virtual void configure_controller() { if (this->controller) this->controller->configure_controller(); } virtual long connect_controller( std::string args, std::string &retstring ) = 0; virtual long disconnect_controller( std::string args, std::string &retstring ) = 0; @@ -128,30 +129,32 @@ namespace Camera { virtual long do_expose() = 0; - /** @brief returns true if cmd is an instrument-specific command - */ - virtual bool is_instrument_command(const std::string &cmd) { return false; } - // Names this instrument handles, so a caller can enumerate rather than // guess. CAMERAD_SYNTAX lists none of them virtual std::vector instrument_commands() const { return {}; } /** @brief returns error if not overridden + * @details The end of the dispatch chain. A name that reaches this + * default is claimed by no handler. */ virtual long instrument_cmd(const std::string &cmd, const std::string &args, std::string &retstring) { + const std::string function("Camera::Interface::instrument_cmd"); + logwrite(function, "ERROR unknown command: "+cmd); retstring="not_supported"; return ERROR; } - /** @brief returns error if not overridden + /** @brief handles commands unique to the controller + * @details The server calls this for every name the core does not + * recognize centrally. A controller interface handles its own + * commands and passes any other name to instrument_cmd. */ virtual long controller_cmd(const std::string &cmd, const std::string &args, std::string &retstring) { - retstring="not_supported"; - return ERROR; + return this->instrument_cmd(cmd, args, retstring); } }; diff --git a/camerad/camera_server.cpp b/camerad/camera_server.cpp index cdfe941..27709aa 100644 --- a/camerad/camera_server.cpp +++ b/camerad/camera_server.cpp @@ -226,14 +226,6 @@ namespace Camera { } ret = HELP; } - /** - * instrument-specific commands - */ - else - if (interface->is_instrument_command(cmd)) - { - ret = interface->instrument_cmd(cmd, args, retstring); - } else if ( cmd == CAMERAD_ABORT ) { ret = interface->abort(args, retstring); @@ -252,7 +244,7 @@ namespace Camera { } else if ( cmd == CAMERAD_BIN ) { - ret = interface->bias(args, retstring); + ret = interface->bin(args, retstring); } else if ( cmd == CAMERAD_CLOSE ) { @@ -280,6 +272,10 @@ namespace Camera { ret = interface->load_firmware(args, retstring); } else + if ( cmd == CAMERAD_MODE ) { + ret = interface->camera_mode(args, retstring); + } + else if ( cmd == CAMERAD_OPEN ) { ret = interface->connect_controller(args, retstring); } @@ -304,60 +300,11 @@ namespace Camera { ret = interface->datacube(args, retstring); } - /** - * controller-specific commands - */ - else - if ( cmd == CAMERAD_LOADTIMING ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == CAMERAD_READACF ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == "getp" ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == "inreg" ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == "setp" ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == CAMERAD_MODE ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == CAMERAD_RAW ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == "bob" ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == "autofetch_mode" ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == CAMERAD_HEATER ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - else - if ( cmd == CAMERAD_SENSOR ) { - ret = interface->controller_cmd(cmd, args, retstring); - } - - // unknown commands generate an error + // any other name goes to the controller interface, which forwards + // what it does not handle to the instrument // else { - message.str(""); message << "ERROR unknown command: " << cmd; - logwrite(function, message.str()); - ret = ERROR; + ret = interface->controller_cmd(cmd, args, retstring); } // If retstring not empty then append "DONE" or "ERROR" depending on value of ret, diff --git a/docs/_ext/camerad_tables.py b/docs/_ext/camerad_tables.py index 50db31a..f6b1a75 100644 --- a/docs/_ext/camerad_tables.py +++ b/docs/_ext/camerad_tables.py @@ -48,13 +48,20 @@ FRAME_OUTPUT_SOURCE = "utils/frame_output_factory.cpp" FRAME_OUTPUT_KEY_PATTERN = re.compile(r'\bkey\s*==\s*"([A-Z][A-Z0-9_]*)"') +# The server hands every name it does not recognize to the controller interface, so the names that +# dispatcher handles are dispatched commands too. Its body runs from the definition to the closing +# banner +CONTROLLER_DISPATCH_SOURCE = "camerad/archon_interface.cpp" +CONTROLLER_DISPATCH_START = "long ArchonInterface::controller_cmd(" +CONTROLLER_DISPATCH_END = "/***** Camera::ArchonInterface::controller_cmd" + # Matched by the dispatch scan but not commands NON_COMMANDS = frozenset({"_EXCEPTION_", "-h", "--help", "help", "?"}) @dataclass(frozen=True) class Command: - """One command the server dispatches, with its advertised syntax.""" + """One command camerad dispatches, with its advertised syntax.""" name: str syntax: str @@ -135,8 +142,7 @@ def _advertised_syntax(header: str, constants: dict[str, str]) -> dict[str, str] return syntax -def _dispatched_commands(server: str, constants: dict[str, str]) -> list[str]: - body = server[server.index("Process commands here"):] +def _matched_commands(body: str, constants: dict[str, str]) -> list[str]: found = [] for constant, literal in re.findall(r'cmd\s*==\s*(?:(CAMERAD_\w+)|"([^"]+)")', body): name = constants[constant] if constant else literal @@ -145,12 +151,23 @@ def _dispatched_commands(server: str, constants: dict[str, str]) -> list[str]: return found +def _dispatched_commands(server: str, controller: str, constants: dict[str, str]) -> list[str]: + """Names the server recognizes, then those the controller interface handles itself.""" + found = _matched_commands(server[server.index("Process commands here"):], constants) + start = controller.index(CONTROLLER_DISPATCH_START) + body = controller[start:controller.index(CONTROLLER_DISPATCH_END, start)] + found += [name for name in _matched_commands(body, constants) if name not in found] + return found + + @cache def load_commands() -> list[Command]: header = _read("common/camerad_commands.h") constants = _command_constants(header) syntax = _advertised_syntax(header, constants) - dispatched = _dispatched_commands(_read("camerad/camera_server.cpp"), constants) + dispatched = _dispatched_commands( + _read("camerad/camera_server.cpp"), _read(CONTROLLER_DISPATCH_SOURCE), constants + ) described = _load_data("commands.yaml") _check_coverage("commands", set(dispatched), set(described)) @@ -329,7 +346,7 @@ def _render(self, headers: list[str], rows: list[list[str]]) -> list[nodes.Node] class CameradCommands(_TableDirective): - """Render the table of commands the server dispatches.""" + """Render the table of commands camerad dispatches.""" option_spec = {"widths": str} diff --git a/docs/architecture/index.md b/docs/architecture/index.md index 91c0918..dca5318 100644 --- a/docs/architecture/index.md +++ b/docs/architecture/index.md @@ -107,15 +107,17 @@ An instrument is a separate repository, checked out as a submodule under 1. A `.cmake` fragment setting `INSTRUMENT_SOURCES`. That is the whole build integration. 2. An `Interface` subclass deriving from `ArchonInterface` or `AstroCamInterface`, overriding what - the detector needs and adding commands through `instrument_cmd()` and `is_instrument_command()`. + the detector needs and adding commands through `instrument_cmd()` and `instrument_commands()`. 3. Its own `ExposureMode` implementations, where the stock ones do not fit. 4. Optionally a FITS header dictionary and shipped `.cfg` and `.acf` files. An interface factory ties it together, so the core builds against the base class and never names a concrete instrument. -Instrument commands are checked *before* the base command chain, so an instrument can add commands -and also override a base one of the same name. +The server recognizes the base commands first, and an instrument changes one by overriding its +method. Any other name goes to `controller_cmd()`, which handles the controller's own commands and +forwards the rest to `instrument_cmd()`. A controller command takes precedence over an instrument +command of the same name. See [instruments](../instruments/index.md) for the modules that exist, and {source}`camerad/Instruments/hispec_tracking_camera` for the most complete example. diff --git a/docs/commands/base.md b/docs/commands/base.md index 9e45507..96e1394 100644 --- a/docs/commands/base.md +++ b/docs/commands/base.md @@ -1,13 +1,15 @@ # Base commands -Every command the server dispatches. Instrument modules add their own on top of these; see +Every command camerad dispatches. Instrument modules add their own on top of these; see [instruments](../instruments/index.md). Any command accepts `?` as its argument to return its own syntax. -The **Controller** column distinguishes commands the interface implements directly from those -routed through `controller_cmd`, which only the Archon interface implements. On an ARC build the -latter return `not_supported`. +The **Controller** column marks the commands only the Archon interface implements. The server +recognizes the core's commands, `mode` among them, and calls the interface's function for each. It +hands any other name to `controller_cmd`, where the Archon interface handles its own commands and +forwards the rest to the instrument. On an ARC build the Archon commands return `not_supported` +unless the instrument claims them. ```{eval-rst} .. camerad-commands:: @@ -15,8 +17,8 @@ latter return `not_supported`. ``` :::{note} -This table is generated from the dispatch chain in {source}`camerad/camera_server.cpp`, so it lists -what the server actually answers. The `help` command is a separate hand-maintained list in +This table is generated from the dispatch chains in {source}`camerad/camera_server.cpp` and +{source}`camerad/archon_interface.cpp`, so it lists what camerad actually answers. The `help` command is a separate hand-maintained list in {source}`common/camerad_commands.h` that has drifted: it advertises around two dozen commands the server no longer implements, and omits several it does, including `power`. Trust this table over `help`. @@ -25,5 +27,5 @@ server no longer implements, and omits several it does, including `power`. Trust ## Syntax not shown A few commands are dispatched but absent from the syntax list, so no argument syntax is generated -for them. They are `power`, `getp`, `setp`, `inreg`, `autofetch_mode` and `bob`. Use `?` against a +for them. They are `power`, `getp`, `setp`, `inreg` and `autofetch_mode`. Use `?` against a running server for the authoritative syntax. diff --git a/docs/commands/controller.md b/docs/commands/controller.md index c54f37d..5879a8c 100644 --- a/docs/commands/controller.md +++ b/docs/commands/controller.md @@ -4,8 +4,9 @@ Commands that reach the detector controller rather than being interpreted by the the escape hatch for anything the higher-level commands do not cover. Two routes exist. `native` passes a command straight through and returns the reply. The commands -routed through `controller_cmd`, marked Archon in the [base command table](base.md), are -server-side implementations of controller-specific features. +marked Archon in the [base command table](base.md) are implemented only by the Archon interface. +The server recognizes `mode` itself; it hands any other name it does not recognize to +`controller_cmd`, which handles the Archon's own commands and forwards the rest to the instrument. ## Archon @@ -38,8 +39,9 @@ Beyond `native`, the Archon-specific commands are `raw` for pre-CDS sample captu `native` is not implemented on an ARC build. `AstroCamInterface::native` logs "not yet implemented" and returns success, so the command replies `DONE` while doing nothing. -`controller_cmd` is not implemented either, so every command in the Archon list above returns -`not_supported` on an ARC build. +`controller_cmd` handles no commands of its own on an ARC build, so the commands in the Archon list +above that the server does not recognize pass to the instrument and return `not_supported` unless +the instrument handles them. ::: Passing three-letter DSP commands through to an ARC controller is therefore not currently possible diff --git a/docs/data/commands.yaml b/docs/data/commands.yaml index 4d17a5f..5a4fc97 100644 --- a/docs/data/commands.yaml +++ b/docs/data/commands.yaml @@ -1,11 +1,11 @@ # Descriptions for the commands camerad dispatches. # # The command list and the argument syntax are generated from the source; this file supplies the -# prose. A command added to or removed from the dispatch in camerad/camera_server.cpp without a -# matching change here fails the documentation build. +# prose. A command added to or removed from the dispatch in camerad/camera_server.cpp or +# ArchonInterface::controller_cmd without a matching change here fails the documentation build. # # summary one line, imperative -# controller "any", or "archon" for commands routed through controller_cmd +# controller "any", or "archon" for commands only the Archon interface implements # note optional caveat appended to the summary abort: @@ -32,12 +32,7 @@ bias: bin: summary: Set or get the binning factor controller: any - note: "Broken: the server routes this to the bias handler, so an argument is applied as a bias rather than a binning factor." - -bob: - summary: Reach the BOB interface - controller: archon - note: "Non-functional: no controller implements a handler for it, so it always returns an error." + note: "Not implemented: the Archon interface logs `not yet implemented` and returns an error." close: summary: Disconnect from the controller @@ -88,7 +83,7 @@ loadtiming: controller: archon mode: - summary: Select a camera mode defined in the ACF + summary: Select a camera mode, a named configuration carried in the ACF controller: archon native: diff --git a/docs/python/index.md b/docs/python/index.md index da024c8..e8bd254 100644 --- a/docs/python/index.md +++ b/docs/python/index.md @@ -54,17 +54,17 @@ putting the build's `lib` directory on `PYTHONPATH`. Every command `camerad` accepts is reachable, through three routes: Base commands -: Bound as methods: `open`, `close`, `load`, `power`, `exptime`, `expose`, `abort`, `key`, +: Bound as methods: `open`, `close`, `load`, `power`, `mode`, `exptime`, `expose`, `abort`, `key`, `datacube` and the rest. Instrument commands : `instrument_cmd(command, args)`, a deliberate passthrough rather than one binding each, so a build whose instrument gains a command exposes it with no change to the module. Enumerate with - `instrument_commands()`, or test one with `is_instrument_command()`. + `instrument_commands()`. Controller commands -: `controller_cmd(command, args)` for `mode`, `raw`, `readacf`, `loadtiming`, `heater`, `sensor` and - the other Archon-only commands. +: `controller_cmd(command, args)` for `raw`, `readacf`, `loadtiming`, `heater`, `sensor` and the + other Archon-only commands. It forwards any other name to the instrument, as camerad does. Only `exit` is missing, since the process belongs to the caller. diff --git a/python/camera_interface_module.cpp b/python/camera_interface_module.cpp index 06a7fd9..1831af9 100644 --- a/python/camera_interface_module.cpp +++ b/python/camera_interface_module.cpp @@ -259,6 +259,15 @@ PYBIND11_MODULE(camera_interface, module) { py::arg("args") = "", py::call_guard(), "Add, list or remove a user FITS header key") + .def("mode", + [](CameraSession &self, const std::string &args) { + std::string retstring; + const long status = (*self).camera_mode(args, retstring); + return invoke(status, "mode", retstring); + }, + py::arg("args"), py::call_guard(), + "Select a camera mode, camerad's named operational configuration") + .def("test", [](CameraSession &self, const std::string &args) { std::string retstring; @@ -267,8 +276,8 @@ PYBIND11_MODULE(camera_interface, module) { py::arg("args") = "", py::call_guard(), "Run a named interface test") - // Covers mode, raw, readacf, loadtiming, heater, sensor and autofetch_mode, - // which camerad also routes through controller_cmd + // Runs a command unique to the controller. A name the controller does not + // own is forwarded to the instrument, as camerad does .def("controller_cmd", [](CameraSession &self, const std::string &command, const std::string &args) { std::string retstring; @@ -287,13 +296,6 @@ PYBIND11_MODULE(camera_interface, module) { py::arg("args"), py::call_guard(), "Send a raw command straight to the controller") - .def("is_instrument_command", - [](CameraSession &self, const std::string &command) { - return (*self).is_instrument_command(command); - }, - py::arg("command"), - "Return True if this build's instrument handles the named command") - .def("instrument_commands", [](CameraSession &self) { return (*self).instrument_commands(); }, "Return the instrument-specific command names this build supports") diff --git a/python/examples/raw_channel_sweep.py b/python/examples/raw_channel_sweep.py index 1f5dd25..69f6437 100644 --- a/python/examples/raw_channel_sweep.py +++ b/python/examples/raw_channel_sweep.py @@ -206,7 +206,7 @@ def run(args: argparse.Namespace) -> int: camera.power("on") if args.keep_config: # After power, because selecting a mode applies CDS settings - camera.controller_cmd("mode", args.mode) + camera.mode(args.mode) if args.exptime is not None: camera.exptime(str(args.exptime)) diff --git a/python/tests/emulator_integration.py b/python/tests/emulator_integration.py index 073c699..606fa84 100644 --- a/python/tests/emulator_integration.py +++ b/python/tests/emulator_integration.py @@ -39,14 +39,11 @@ def check_build_identity(expected_instrument: str) -> None: def check_introspection(camera: camera_interface.Camera) -> None: - """Confirm every enumerated instrument command resolves, and no other does.""" + """Confirm the enumerated instrument commands include no unknown name.""" commands = camera.instrument_commands() print(f"instrument commands: {commands}") - for name in commands: - check(camera.is_instrument_command(name), - f"{name} was enumerated but is_instrument_command says no") - check(not camera.is_instrument_command("nosuchcommand"), - "an unknown command was accepted as an instrument command") + check("nosuchcommand" not in commands, + "an unknown command was enumerated as an instrument command") def check_exptime_units(camera: camera_interface.Camera) -> None: diff --git a/reference_instrument/README.md b/reference_instrument/README.md index b6c3a41..e916549 100644 --- a/reference_instrument/README.md +++ b/reference_instrument/README.md @@ -11,9 +11,6 @@ It derives from `ArchonInterface` and overrides the three specialization points, | Instrument configuration | `configure_instrument` | reads `REFERENCE_VALUE` from the cfg, which `refvalue` reports | | Mode selection | `get_exposure_modes`, `set_exposure_mode` | `exposuremode ?` lists `REFERENCE`; `exposuremode REFERENCE` selects it | -`is_instrument_command` is also overridden, only because the current server consults it before every command. That -is not the design, and the dispatch fix removes it. - `REFERENCE` reads out as `SINGLE` and differs only in the type it reports. Any other mode is constructed by `ArchonInterface`. diff --git a/reference_instrument/reference_instrument.cpp b/reference_instrument/reference_instrument.cpp index f5b2d9b..9449ab6 100644 --- a/reference_instrument/reference_instrument.cpp +++ b/reference_instrument/reference_instrument.cpp @@ -8,8 +8,6 @@ #include "reference_exposure_modes.h" #include "utilities.h" -#include - namespace Camera { /***** Camera::ReferenceInstrument::configure_instrument ********************/ @@ -32,23 +30,6 @@ namespace Camera { /***** Camera::ReferenceInstrument::configure_instrument ********************/ - /***** Camera::ReferenceInstrument::is_instrument_command *******************/ - /** - * @brief returns true if cmd is one of instrument_commands() - * @details The server consults this before every core command, so - * refvalue is reached only through it. That gate is not the - * design: a name the core does not recognize is meant to pass - * to the controller and then to the instrument with no gate. - * This override goes when the server does that. - * - */ - bool ReferenceInstrument::is_instrument_command(const std::string &cmd) { - const auto commands = this->instrument_commands(); - return std::find(commands.begin(), commands.end(), cmd) != commands.end(); - } - /***** Camera::ReferenceInstrument::is_instrument_command *******************/ - - /***** Camera::ReferenceInstrument::instrument_commands *********************/ /** * @brief names of the commands this instrument handles @@ -76,8 +57,7 @@ namespace Camera { return this->refvalue(args, retstring); } else { - retstring="unrecognized command"; - return ERROR; + return this->Interface::instrument_cmd(cmd, args, retstring); } } /***** Camera::ReferenceInstrument::instrument_cmd **************************/ diff --git a/reference_instrument/reference_instrument.h b/reference_instrument/reference_instrument.h index 9655a5d..b9e3710 100644 --- a/reference_instrument/reference_instrument.h +++ b/reference_instrument/reference_instrument.h @@ -18,7 +18,6 @@ namespace Camera { public: void configure_instrument() override; - bool is_instrument_command(const std::string &cmd) override; std::vector instrument_commands() const override; long instrument_cmd(const std::string &cmd, const std::string &args, From 406fdc99ec1a5df5cc381f1c55b9d0dae6279179 Mon Sep 17 00:00:00 2001 From: David Hale Date: Tue, 20 Oct 2026 10:15:44 -0700 Subject: [PATCH 2/2] acfmode becomes an Archon controller command and mode leaves the core * camera_mode is removed from the base. the Archon's mode is renamed acfmode and handled in ArchonInterface::controller_cmd, so the server names no mode * selecting an ACF mode section no longer selects an exposure mode, and default_exposure_mode_name is removed. every load leaves no exposure mode selected, and expose is refused until one is selected * the Python mode binding is removed. the example and the module test select SINGLE after power on * CI checks acfmode through controller_cmd, that acfmode leaves the exposure mode alone, and that a load leaves none selected * breaks HISPEC tracking camera: its default_exposure_mode_name override and two-argument set_camera_mode calls no longer compile --- .github/workflows/emulator-integration.yml | 28 +++++++++++----- README.md | 2 +- camerad/archon_interface.cpp | 39 +++++++++++----------- camerad/archon_interface.h | 5 +-- camerad/camera_interface.h | 1 - camerad/camera_server.cpp | 4 --- common/camerad_commands.h | 4 +-- docs/commands/base.md | 2 +- docs/commands/controller.md | 4 +-- docs/data/commands.yaml | 8 ++--- docs/python/index.md | 2 +- python/camera_interface_module.cpp | 9 ----- python/examples/raw_channel_sweep.py | 8 +++-- python/tests/emulator_integration.py | 2 ++ 14 files changed, 58 insertions(+), 60 deletions(-) diff --git a/.github/workflows/emulator-integration.yml b/.github/workflows/emulator-integration.yml index a4e9af0..a82c142 100644 --- a/.github/workflows/emulator-integration.yml +++ b/.github/workflows/emulator-integration.yml @@ -148,11 +148,12 @@ jobs: echo "$resp" | grep -q "$2" || { echo "expected \"$2\""; exit 1; } } - expect "open" "DONE" - expect "load" "DONE" - expect "power on" "DONE" - expect "exptime 0.1" "DONE" - expect "expose 1" "DONE" + expect "open" "DONE" + expect "load" "DONE" + expect "power on" "DONE" + expect "exposuremode SINGLE" "^SINGLE DONE" + expect "exptime 0.1" "DONE" + expect "expose 1" "DONE" # instrument command, replying with the value configure_instrument() read expect "refvalue" "^reference-cfg-value DONE" @@ -163,14 +164,25 @@ jobs: expect "exposuremode REFERENCE" "^REFERENCE DONE" expect "expose 1" "DONE" expect "exposuremode" "^REFERENCE DONE" + + # applying an ACF mode section leaves the exposure mode alone + expect "acfmode DEFAULT" "^DONE" + expect "exposuremode" "^REFERENCE" + expect "exposuremode SINGLE" "^SINGLE DONE" expect "expose 1" "DONE" - # mode is a core function; getp is the Archon interface's own command, - # reached through controller_cmd; a name nobody claims is an error - expect "mode DEFAULT" "^DONE" + # acfmode and getp are the Archon interface's own commands, reached + # through controller_cmd; a name nobody claims is an error + expect "acfmode DEFAULT" "^DONE" expect "getp nRowsQuad" "^[0-9]" expect "nosuchcommand" "^not_supported ERROR" + # a load leaves no exposure mode selected, so an exposure is refused + expect "load" "DONE" + expect "power on" "DONE" + expect "exposuremode" "^undefined ERROR" + expect "expose 1" "^exposure mode not set" + - name: Reference module import test run: ctest --test-dir build-reference --output-on-failure diff --git a/README.md b/README.md index 674949a..3576502 100644 --- a/README.md +++ b/README.md @@ -261,7 +261,7 @@ camera.instrument_cmd("roi", "51 60 51 60") print(camera.output_status()) # frames written, dropped, last file ``` -Every command `camerad` accepts is reachable: the base commands, `mode` among them, are bound as methods, instrument-specific ones go through `instrument_cmd()` (enumerable with `instrument_commands()`), and controller-specific ones such as `raw`, `readacf`, `loadtiming`, `heater` and `sensor` go through `controller_cmd()`, which forwards any other name to the instrument as `camerad` does. Only `exit` is omitted, since the process belongs to the caller. +Every command `camerad` accepts is reachable: the base commands are bound as methods, instrument-specific ones go through `instrument_cmd()` (enumerable with `instrument_commands()`), and controller-specific ones such as `raw`, `readacf`, `loadtiming`, `heater` and `sensor` go through `controller_cmd()`, which forwards any other name to the instrument as `camerad` does. Only `exit` is omitted, since the process belongs to the caller. The controller and instrument are fixed at CMake configure time, so `instrument_name()` and `controller_name()` report which build was loaded. A failed command raises `RuntimeError`. diff --git a/camerad/archon_interface.cpp b/camerad/archon_interface.cpp index 7c0a9db..b8441ea 100644 --- a/camerad/archon_interface.cpp +++ b/camerad/archon_interface.cpp @@ -101,6 +101,10 @@ namespace Camera { return this->set_parameter(args, retstring); } else + if ( cmd == CAMERAD_ACFMODE ) { + return this->acfmode(args, retstring); + } + else if ( cmd == CAMERAD_RAW ) { return this->raw(args, retstring); } @@ -716,6 +720,10 @@ namespace Camera { const std::string function("Camera::ArchonInterface::load_firmware"); logwrite(function, acffile); + // clear the exposure mode, so that none is selected after a load + // + this->exposuremode.reset(); + // load the ACF file and write to Archon configuration memory // long error = this->controller->load_acf(acffile); @@ -729,7 +737,7 @@ namespace Camera { // if (error != NO_ERROR) this->controller->fetchlog(); - // set the mode to DEFAULT + // set the acfmode to DEFAULT // if (error == NO_ERROR) error = this->set_camera_mode(std::string("DEFAULT")); @@ -796,27 +804,24 @@ namespace Camera { /***** Camera::ArchonInterface::load_timing *********************************/ - /***** Camera::ArchonInterface::camera_mode *********************************/ + /***** Camera::ArchonInterface::acfmode *************************************/ /** - * @brief set a camera "mode" - * @details Allowable camera modes are defined in the ACF file and specify - * a set of camera settings which can be used to change camera - * settings. - * This version fits the general description of all virtual functions - * inherited by the Camera::Interface base class and is overridden - * by an Archon-specific implementation. + * @brief applies an ACF mode section directly + * @details Applies the settings in the ACF file's [MODE_] section. + * It does not select an exposure mode. * @param[in] args requested mode name (case-insensitive) * @param[out retstring current mode name * @return ERROR|NO_ERROR * */ - long ArchonInterface::camera_mode( const std::string args, std::string &retstring ) { - const std::string function("Camera::ArchonInterface::camera_mode"); + long ArchonInterface::acfmode(const std::string &args, std::string &retstring) { + const std::string function("Camera::ArchonInterface::acfmode"); // Help if (args=="?" || args=="help") { - retstring = CAMERAD_MODE; + retstring = CAMERAD_ACFMODE; retstring.append( " \n" ); - retstring.append( " Applies camera settings associated with MODE_ specified in the ACF file.\n"); + retstring.append( " Applies the settings of [MODE_] in the ACF file.\n" ); + retstring.append( " Does not select an exposure mode.\n" ); retstring.append( " Valid modes are: {" ); for (const auto &mode : this->controller->modemap) { retstring.append(" "); retstring.append(mode.first); @@ -827,7 +832,7 @@ namespace Camera { // call the work function return( this->set_camera_mode(args) ); } - /***** Camera::ArchonInterface::camera_mode *********************************/ + /***** Camera::ArchonInterface::acfmode *************************************/ /***** Camera::ArchonInterface::set_camera_mode *****************************/ @@ -873,12 +878,6 @@ namespace Camera { // if we made it all the way to the end then this is the selected mode this->controller->selectedmode = modeselect; - std::string target = this->default_exposure_mode_name(); - for (const auto &m : this->get_exposure_modes()) { - if (m == modeselect) { target = modeselect; break; } - } - this->set_exposure_mode(target, {}); - return NO_ERROR; } /***** Camera::ArchonInterface::set_camera_mode *****************************/ diff --git a/camerad/archon_interface.h b/camerad/archon_interface.h index 56126a6..89e7caa 100644 --- a/camerad/archon_interface.h +++ b/camerad/archon_interface.h @@ -32,7 +32,6 @@ namespace Camera { long basename( const std::string args, std::string &retstring ) override; long bias( const std::string args, std::string &retstring ) override; long bin( const std::string args, std::string &retstring ) override; - long camera_mode( const std::string args, std::string &retstring ) override; long connect_controller( const std::string args, std::string &retstring ) override; long disconnect_controller( const std::string args, std::string &retstring ) override; long exptime( const std::string args, std::string &retstring ) override; @@ -72,9 +71,7 @@ namespace Camera { long heater(const std::string &args, std::string &retstring); long sensor(const std::string &args, std::string &retstring); long autofetch_mode(const std::string &args, std::string &retstring); - - // Fallback for set_camera_mode when the camera-mode name is unknown - virtual std::string default_exposure_mode_name() const { return "SINGLE"; } + long acfmode(const std::string &args, std::string &retstring); char* get_framebuf() { return controller->framebuf; } diff --git a/camerad/camera_interface.h b/camerad/camera_interface.h index d5dba78..30cdc18 100644 --- a/camerad/camera_interface.h +++ b/camerad/camera_interface.h @@ -112,7 +112,6 @@ namespace Camera { virtual long basename( std::string args, std::string &retstring ) = 0; virtual long bias( std::string args, std::string &retstring ) = 0; virtual long bin( std::string args, std::string &retstring ) = 0; - virtual long camera_mode( std::string args, std::string &retstring ) = 0; ///< selects a mode, camerad's named operational configuration virtual void configure_controller() { if (this->controller) this->controller->configure_controller(); } virtual long connect_controller( std::string args, std::string &retstring ) = 0; virtual long disconnect_controller( std::string args, std::string &retstring ) = 0; diff --git a/camerad/camera_server.cpp b/camerad/camera_server.cpp index 27709aa..8341ee7 100644 --- a/camerad/camera_server.cpp +++ b/camerad/camera_server.cpp @@ -272,10 +272,6 @@ namespace Camera { ret = interface->load_firmware(args, retstring); } else - if ( cmd == CAMERAD_MODE ) { - ret = interface->camera_mode(args, retstring); - } - else if ( cmd == CAMERAD_OPEN ) { ret = interface->connect_controller(args, retstring); } diff --git a/common/camerad_commands.h b/common/camerad_commands.h index 639358b..124b966 100644 --- a/common/camerad_commands.h +++ b/common/camerad_commands.h @@ -9,6 +9,7 @@ #pragma once const std::string CAMERAD_ABORT("abort"); +const std::string CAMERAD_ACFMODE("acfmode"); const std::string CAMERAD_AUTODIR("autodir"); const std::string CAMERAD_BASENAME("basename"); const std::string CAMERAD_BIAS("bias"); @@ -39,7 +40,6 @@ const std::string CAMERAD_LOADTIMING("loadtiming"); const std::string CAMERAD_LONGERROR("longerror"); const std::string CAMERAD_MEX("mex"); const std::string CAMERAD_MEXAMPS("mexamps"); -const std::string CAMERAD_MODE("mode"); const std::string CAMERAD_MODEXPTIME("modexptime"); const std::string CAMERAD_NATIVE("native"); const std::string CAMERAD_OPEN("open"); const int CAMERAD_OPEN_TIMEOUT(10000); @@ -58,6 +58,7 @@ const std::string CAMERAD_USEFRAMES("useframes"); const std::string CAMERAD_WRITEKEYS("writekeys"); const std::vector CAMERAD_SYNTAX = { CAMERAD_ABORT, + CAMERAD_ACFMODE+" [ ? | ]", CAMERAD_AUTODIR, CAMERAD_BASENAME, CAMERAD_BIAS, @@ -88,7 +89,6 @@ const std::vector CAMERAD_SYNTAX = { CAMERAD_LONGERROR, CAMERAD_MEX, CAMERAD_MEXAMPS, - CAMERAD_MODE+" [ ? | ]", CAMERAD_MODEXPTIME, CAMERAD_NATIVE+" ? | [|] [ [ < ARG2> [ [ ] ] ] ]", CAMERAD_OPEN+" [ ? | ]", diff --git a/docs/commands/base.md b/docs/commands/base.md index 96e1394..bb5c878 100644 --- a/docs/commands/base.md +++ b/docs/commands/base.md @@ -6,7 +6,7 @@ Every command camerad dispatches. Instrument modules add their own on top of the Any command accepts `?` as its argument to return its own syntax. The **Controller** column marks the commands only the Archon interface implements. The server -recognizes the core's commands, `mode` among them, and calls the interface's function for each. It +recognizes the core's commands and calls the interface's function for each. It hands any other name to `controller_cmd`, where the Archon interface handles its own commands and forwards the rest to the instrument. On an ARC build the Archon commands return `not_supported` unless the instrument claims them. diff --git a/docs/commands/controller.md b/docs/commands/controller.md index 5879a8c..8d9066d 100644 --- a/docs/commands/controller.md +++ b/docs/commands/controller.md @@ -5,7 +5,7 @@ the escape hatch for anything the higher-level commands do not cover. Two routes exist. `native` passes a command straight through and returns the reply. The commands marked Archon in the [base command table](base.md) are implemented only by the Archon interface. -The server recognizes `mode` itself; it hands any other name it does not recognize to +The server hands any name it does not recognize centrally to `controller_cmd`, which handles the Archon's own commands and forwards the rest to the instrument. ## Archon @@ -30,7 +30,7 @@ The server itself parses `FRAME`, `STATUS` and `SYSTEM` replies for its own book Beyond `native`, the Archon-specific commands are `raw` for pre-CDS sample capture, `getp` and `setp` for parameters, `inreg` for a VCPU input register, `loadtiming` and `readacf` for loading, -`mode` for camera modes, `autofetch_mode`, and `heater` and `sensor` for the thermal modules. The +`acfmode` for ACF mode sections, `autofetch_mode`, and `heater` and `sensor` for the thermal modules. The [raw samples](#raw-samples) and [heater and sensor](#heater-and-sensor) syntax is below. ## ARC (AstroCam) diff --git a/docs/data/commands.yaml b/docs/data/commands.yaml index 5a4fc97..d64ede6 100644 --- a/docs/data/commands.yaml +++ b/docs/data/commands.yaml @@ -12,6 +12,10 @@ abort: summary: Abort an exposure in progress controller: any +acfmode: + summary: Apply an ACF mode section directly + controller: archon + autodir: summary: Write images into a dated subdirectory controller: any @@ -82,10 +86,6 @@ loadtiming: summary: Load a timing script and its parameters controller: archon -mode: - summary: Select a camera mode, a named configuration carried in the ACF - controller: archon - native: summary: Send a command straight to the controller and return its reply controller: any diff --git a/docs/python/index.md b/docs/python/index.md index e8bd254..cb43886 100644 --- a/docs/python/index.md +++ b/docs/python/index.md @@ -54,7 +54,7 @@ putting the build's `lib` directory on `PYTHONPATH`. Every command `camerad` accepts is reachable, through three routes: Base commands -: Bound as methods: `open`, `close`, `load`, `power`, `mode`, `exptime`, `expose`, `abort`, `key`, +: Bound as methods: `open`, `close`, `load`, `power`, `exptime`, `expose`, `abort`, `key`, `datacube` and the rest. Instrument commands diff --git a/python/camera_interface_module.cpp b/python/camera_interface_module.cpp index 1831af9..84da7a2 100644 --- a/python/camera_interface_module.cpp +++ b/python/camera_interface_module.cpp @@ -259,15 +259,6 @@ PYBIND11_MODULE(camera_interface, module) { py::arg("args") = "", py::call_guard(), "Add, list or remove a user FITS header key") - .def("mode", - [](CameraSession &self, const std::string &args) { - std::string retstring; - const long status = (*self).camera_mode(args, retstring); - return invoke(status, "mode", retstring); - }, - py::arg("args"), py::call_guard(), - "Select a camera mode, camerad's named operational configuration") - .def("test", [](CameraSession &self, const std::string &args) { std::string retstring; diff --git a/python/examples/raw_channel_sweep.py b/python/examples/raw_channel_sweep.py index 69f6437..3ee0178 100644 --- a/python/examples/raw_channel_sweep.py +++ b/python/examples/raw_channel_sweep.py @@ -204,9 +204,11 @@ def run(args: argparse.Namespace) -> int: else: camera.load() camera.power("on") + # Neither load nor readacf selects an exposure mode + camera.exposure_mode("SINGLE") if args.keep_config: - # After power, because selecting a mode applies CDS settings - camera.mode(args.mode) + # After power, because an ACF mode section applies CDS settings + camera.controller_cmd("acfmode", args.mode) if args.exptime is not None: camera.exptime(str(args.exptime)) @@ -244,7 +246,7 @@ def main() -> int: help="leave the controller's loaded configuration alone, " "reading the ACF host-side only") parser.add_argument("--mode", default="DEFAULT", - help="camera mode to select with --keep-config " + help="ACF mode section to apply with --keep-config " "(default: DEFAULT)") parser.add_argument("--exptime", type=float, help="exposure time, in the unit the config selects") diff --git a/python/tests/emulator_integration.py b/python/tests/emulator_integration.py index 606fa84..d5861db 100644 --- a/python/tests/emulator_integration.py +++ b/python/tests/emulator_integration.py @@ -118,6 +118,8 @@ def run(config_path: str, fits_dir: pathlib.Path, expected_instrument: str, camera.open() camera.load() camera.power("on") + # A load selects no exposure mode + camera.exposure_mode("SINGLE") check_exptime_units(camera)