Repository navigation
Conversation
|
@BrianJKoopman or @YutaroKASAI, please review my update. We cannot read the angles if this PR was deployed to the site software. |
There was a problem hiding this comment.
Thanks for the PR, some minor comments below. Do you have plans to test against the real hardware before we merge?
I'm also wondering about the state of the sherborne sensors. It seems like the temperature readout doesn't really work as is, and it was noted that we'd likely drop support for them. Should we do that now? Or are they still in use?
EDIT: Also, please merge in the latest from the main branch. That will fix the checks.
| The tilt sensor data is sent via serial-to-ethernet converter. | ||
| The converter is linked to the tilt sensor | ||
| via RS-422(DWL) or RS-485(sherborne), D-sub 9-pin cable. | ||
| via RS-422(DWL), RS-485(sherborne) or RS-232 (SLP2). |
There was a problem hiding this comment.
Is it still a 9-pin D-sub cable? The RS-232/RS-422/RS-485 just defines the serial standard.
There was a problem hiding this comment.
The type of cable is different depending on tilt sensor.
So, I removed the words.
There was a problem hiding this comment.
I like the added info. Let's move the more detailed info starting from "There are three types of tilt sensors..." down to a "Description" section below the "Docker Compose" section/above the "Agent API" section at the ------- level. (See this example for location reference.)
| 'Could not start acq because {} is already running' | ||
| ) | ||
| msg = f'Could not start acq because {self.lock.job} is already running' | ||
| self.log.warn(msg) |
There was a problem hiding this comment.
Put this at the 'error' level, since it stops the process:
| self.log.warn(msg) | |
| self.log.error(msg) |
| tiltsensor_data['data']['temperatureX'] = -999 | ||
| tiltsensor_data['data']['temperatureY'] = -999 |
There was a problem hiding this comment.
The example session data says these temperature fields are only available on the 'sherborne' sensors. Does this mean 'DWL' has no temperature reading? Should we also have a check here for elif self.sensor_type == 'sherborne'?
There was a problem hiding this comment.
Also, below it looks like 'temperatureX' and 'temperatureY' can't be updated -- all the 'sherbone' code is commented out. Why initialize this at all?
| return False | ||
| msg = ("Lock could not be acquired because it " | ||
| + f"is held by {self.lock.job}") | ||
| self.log.warn(msg) |
There was a problem hiding this comment.
Same as above:
| self.log.warn(msg) | |
| self.log.error(msg) |
| tcp_port (int): TCP port | ||
| """ | ||
| if tcp_ip is None or tcp_port is None: | ||
| raise Exception( |
There was a problem hiding this comment.
This could be more specific. I'd recommend using TypeError instead of the general Exception.
| "Aborted SLP2._conn() due to no " | ||
| "TCP port specified") |
There was a problem hiding this comment.
The error message says no port was specified, but it can also trigger if no IP was provided.
| else: | ||
| raise Exception( | ||
| "Aborted SLP2._conn() due to unknown error") |
There was a problem hiding this comment.
I don't think this ever evaluates. You could simplify the conditionals here.
for more information, see https://pre-commit.ci
Description
Add support for the Pacico SLP2 inclinometer to the Wiregrid Tilt Sensor Agent, and fix several existing error-handling bugs in the agent.
New SLP2 support
socs/agents/wiregrid_tiltsensor/drivers/slp2.py(classSLP2), communicating over the serial-to-ethernet converter with ASCII commands:stpcasc(stop continuous output) andsetoasc(ASCII output mode).get_angle():get-x&y, returns(X, Y)in degrees.get_temp():gettemp, returns one temperature in degC. ADC bytes may precede the value in the reply, so the reply is read until a CR that follows a number, and the last number is used.setzcur/setzfac) are intentionally not implemented, since they change the sensor's stored state.--sensor-type slp2option.slp2, the agent records onetemperaturefield in thewgtiltsensorfeed andsession.data, because the SLP2 has a single temperature sensor common to both axes. ForDWL/sherborne, the feed is unchanged (temperatureX/temperatureY).Bug fixes in the existing agent
connect()raised a string (raise ('Invalid tiltsensor type')), which itself fails withTypeError. It now raisesValueError.--sensor-typealso haschoices=['DWL', 'sherborne', 'slp2'], so typos are rejected at startup.acqonly logged a warning when the lock could not be acquired and then kept running. It now returnsFalse. The log message also now includes the lock holder (it used to print a literal{}).acqstopped on network errors (OSError, e.g. connection refused or broken pipe), and also stopped if reconnecting failed. NowOSErroris caught, the connection is closed, and the agent reconnects at the start of the next loop, retrying until it succeeds.resetreturned a bareFalsewhen the lock could not be acquired. It now returns(False, msg). It also returnsFalseif the sensor is currently disconnected.Docs
docs/agents/wiregrid_tiltsensor.rst: SLP2 description,temperaturefield, reconnection behaviour, an SLP2 site-config example, and some typo fixes.AI-assisted, and added# Code developed with AI assistance.to the modified and new source files, per CONTRIBUTING.rst.Motivation and Context
We are upgrading the wire-grid tilt sensor to a Pacico SLP2, which the agent did not support. The error-handling bugs above were found while reviewing the agent for this change. The
OSErrorfix in particular matters for unattended operation, because a dropped connection to the converter used to stop data acquisition.How Has This Been Tested?
Checklist:
AI Usage Disclosure
Claude Code (Claude Opus 5.5) was used to write the SLP2 driver, the agent changes, the bug fixes and the documentation updates, based on a standalone SLP2 test script I wrote. I reviewed the changes. The files are marked with
# Code developed with AI assistance.and the docs page has theAI-assistedbadge.