Review done by Qwen3.8 (code was written by another model)
I've finished a thorough review of the whole tree (the ai_slop2 branch: the C→C++ pmacAsynIPPort rewrite, the sinqMotor submodule, turboPmacController/turboPmacAxis, db/Makefile, and utils). Here's what I found, most severe first.
1. High — doFlushHardware() can dereference a null pointer (crash)
src/turboPmacController.cpp:468-482
asynStatusturboPmacController::doFlushHardware(){inttemp=pTurboPmacC_->pasynInt32SyncIOipPort->reason;// <-- no null check
pasynInt32SyncIOipPort is a member, but it is only ever declared in turboPmacController; it is the sinqController base constructor that populates its own pasynOctetSyncIOipPort. The derived class declares its own pasynInt32SyncIOipPort (initialized to nullptr) and calls pasynInt32SyncIO->connect(ipPortConfigName, 0, &pTurboPmacC_->pasynInt32SyncIOipPort, NULL) at line 169 — but then checks the stale local status (line 171), which still holds the value from callParamCallbacks(), not the return value of connect(). So:
If connect() fails, the IOC does not exit — it continues with pasynInt32SyncIOipPort == nullptr, and the first FLUSH_HARDWARE PV write segfaults.
Even in the success case this is fragile: connect()'s real status is discarded.
Fix: status = pasynInt32SyncIO->connect(...); if (status != asynSuccess || ...ptr == nullptr) exit(-1); and add a null guard in doFlushHardware().
2. High — same latent bug in the sinqController constructor
sinqMotor/src/sinqController.cpp:154,167-175
asynStatus status = asynSuccess; (line 154) is never assigned by pasynOctetSyncIO->connect(...) (line 167), so the check at line 169 (status != asynSuccess || ptr == nullptr) never fires on the status branch. A failed connect is only caught by the nullptr half — and only if connect() leaves the pointer null. The intended "exit IOC on fatal connect failure" is effectively broken.
3. High — readResponse() / readOctet() can consume up to 2× the full response
src/pmacAsynIPPort.cpp:523-575 and 748-831
inBuf_ is treated as if it were always empty before each socketRead:
readResponse() is called from readOctet()again mid-loop (line 814) whenever inBuf_ was drained, to fetch the rest of a long response. Each call does a fresh recv() and overwrites inBuf_, resetting head/tail. For a PMAC response longer than INPUT_SIZE (1493 bytes) split across several recv()s, the chunks read between readResponse() calls are thrown away — the client gets a response with missing middle data, and writeRead()'s carriage-return count check will then fail with a misleading "unexpected response" error. The original DLS code had the same structure, but there the asyn IP layer buffered between reads; here the socket is read directly, so data lost to the recv() window that was not read is simply gone.
Also note readOctet() calls pmacReadReady() (a full request/response exchange on the socket) before each refill (line 808-812), even though readResponse() does the exact same thing internally — doubling the round-trips and, worse, the READREADY reply bytes can interleave with the real response data.
4. High — flushOctet() leaves inBuf_ holding just-read data, and returns it to the next reader
src/pmacAsynIPPort.cpp:833-855
flushOctet() discards bytes with recv() into a throwaway cbuf, but does not reset inBufHead_/inBufTail_. Any bytes already sitting in inBuf_ (read by a prior readResponse() and not yet consumed by readOctet()) are not flushed — they will be returned by the next readOctet(). If the intent is "toss everything," the buffer must be reset too:
inBufTail_=0;inBufHead_=0;
(pmacFlush() does reset them at line 653-654, which shows the inconsistency.)
5. High — socketRead() can return success with thisRead still uninitialized
src/pmacAsynIPPort.cpp:362-419
thisRead is declared uninitialized (line 364). If pollSocket() returns asynSuccess but pollstatus == 0 (poll timed out, no data), recv() is still called on a non-blocking socket and returns -1/EAGAIN — handled, fine. But if pollstatus > 0 and recv() then returns -1 with another errno that is neither EWOULDBLOCK/EINTR (e.g. ECONNRESET after closeConnection sets aside), status becomes asynError and the early path at 401-402 sets it — OK. The real hole: after the if (thisRead < 0) {...} block, if status == asynError was set but the code falls through, *nbytesRead = thisRead with thisRead = 0 (line 413 clamps) — that's actually fine. However if pollSocket() itself fails (returns asynError) the function returns early (line 388) — fine. The genuinely bad case: pollstatus == 0 but recv() is not called... it always is. On re-inspection the uninitialized read is masked; downgrade this to a style/robustness note: initialize thisRead = 0.
6. Medium — writeRead() counts CRs in a buffer that is not NUL-terminated by strlen safely
response is only NUL-terminated up to the first NUL. readOctet() can return binary-ish PMAC data (STX/BELL/ACK bytes) and the "response" is counted by \r. If the PMAC response contains a NUL byte (possible in binary modes, and Ixx28/Qxx00 can in principle), strlen truncates the count. More concretely: pasynOctetSyncIO->writeRead() with EOS=\006 returns nbytesIn — use nbytesIn (and scan that many bytes) instead of strlen().
7. Medium — format string: size_t printed with %ld
sinqMotor/src/sinqController.cpp:566-571 and 575-578
"More than %ld communication timeouts in %ld seconds.%s\n",...pSinqC_->maxNumberTimeouts,pSinqC_->comTimeoutWindow,...
maxNumberTimeouts is size_t and comTimeoutWindow is time_t — %ld is only correct for long. On platforms where size_t is 32-bit (or time_t differs) this is UB. Use %zu / cast to time_t explicitly.
8. Medium — FFI wrappers don't return after a null dynamic_cast
if(axis==nullptr){errlogPrintf(...);}// <-- no return here
returnaxis->setWatchdogEnabled(enable!=0);// NULL deref
setWatchdogEnabled and setOffsetMovTimeout log and then fall through to dereferencing axis when the cast fails (only setScaleMovTimeout at line 859-863 has the correct return asynError;). Add the missing return asynError;.
Also in setOffsetMovTimeout (line 830): the iocsh arg is registered as iocshArgInt (offsetMovTimeoutArg2) but the FFI takes a double and the caller passes args[2].ival into a double parameter — works by implicit conversion, but inconsistent with setScaleMovTimeout which uses iocshArgDouble.
9. Medium — msgPrintControlKey uses char* (non-const) as a public API type
controller/portName are const char* everywhere in the EPICS API. Every caller is forced to either hold a non-const buffer or cast. This is a design wart, not a memory bug, but it will cause -Werror friction and invites const_cast abuse. Change to const char* (the member is a std::string).
10. Medium — startMovTimeoutWatchdog() computes the timeout from stale/zero fields
sinqMotor/src/sinqAxis.cpp:595-674
motorPosition(&motorPos) is called at line 620, but pl_status is then re-tested at lines 644 and 656 as if it were still the position read — it is, but the pattern is confusing; more importantly, targetPosition is only set in sinqAxis::move(), not in doMove() overrides that set it themselves. If a derived doMove starts a move without going through sinqAxis::move(), targetPosition is 0 and the watchdog deadline is wrong.
timeAccel = 2 * std::ceil(motorVelocity / motorAccel) (line 663) — motorAccel here is motorVelocityRec / motorAccelRec (line 660), i.e. a time, so motorVelocity / motorAccel is not a time; the formula 2*v/a expects a in units/s². This looks dimensionally wrong and will under/over-estimate the accel phase. Worth checking against the intended physics.
11. Medium — enable() can return asynSuccess without the axis actually reaching the requested state
src/turboPmacAxis.cpp:1186-1193
if((axStatus!=-3)==on){..."Axis is already %s."...returnasynSuccess;}
axStatus here is read from pTurboPmacA_->axisStatus, which is only updated by doPoll()/init(). If the axis was deactivated (−5) or in E‑stop (−4) after the last poll, axStatus is stale, so the "already enabled" fast path can misfire and skip the enable command. The doPoll(&moving) at line 1159 updates pTurboPmacA_->axisStatus only if the poll's read of P%2.2d00 succeeds and is parsed — if writeRead fails inside that poll, axisStatus keeps its old value. Consider reading the status directly here instead of trusting the cached member.
12. Low — connectIt() sets fd_ after setsockopt, leaking the fd on the failure path
src/pmacAsynIPPort.cpp:289-301
If setsockopt(TCP_NODELAY) fails, the socket is destroyed — fine. But between connect() success (line 249) and fd_ = fd (line 300), an exception/early return would leak. More usefully: connect() itself (line 315) sets inBufTail_/inBufHead_ = 0after a successful connect, but if the PMAC already has queued data from a previous session it is read into a fresh buffer — acceptable, but combined with #4 the flush-on-reconnect story is inconsistent.
13. Low — pollSocket() ignores the pollmsec < 0 (infinite) EINTR time check
When pollmsec is negative (infinite wait), the comparison > pollmsec is always true on the first EINTR, so the loop breaks immediately and treats an interrupted infinite poll as "done" (pollstatus stays -1). The caller then proceeds to recv()/send() on a possibly-not-ready socket. Guard with if (pollmsec >= 0 && elapsed > pollmsec).
14. Low — dead / misleading members
turboPmacControllerImpl::lastResponse[MAXBUF_] (src/turboPmacController.cpp:48) is written nowhere — dead.
asynStatus errorStatus shadowing in turboPmacAxis::doPoll (line 577) is fine, but the variable nvals/motorRecResolution at line 307 in doPoll is read via getAxisParamChecked but never used afterward — unused (would trip -Wunused-but-set-variable-style scrutiny under -Werror; it currently compiles only because it's "used" by the macro call).
Summary of the top fixes I'd do first
turboPmacController ctor: capture the real return of pasynInt32SyncIO->connect() and exit on failure; null-guard in doFlushHardware() (#1).
sinqController ctor: same fix for pasynOctetSyncIO->connect() (#2).
Reset inBuf_ in flushOctet() and make readResponse()/readOctet() handle a partially-filled inBuf_ instead of overwriting it (#3, #4).
Add the missing return asynError; in the two sinqAxis FFI wrappers (#8).
Scan nbytesIn bytes instead of strlen(response) in writeRead() (#6).
Want me to apply any of these fixes? I'd suggest starting with #1–#2 (crash safety) since they're small, self-contained, and unambiguously correct.
Review done by Qwen3.8 (code was written by another model)
I've finished a thorough review of the whole tree (the `ai_slop2` branch: the C→C++ `pmacAsynIPPort` rewrite, the sinqMotor submodule, `turboPmacController`/`turboPmacAxis`, db/Makefile, and utils). Here's what I found, most severe first.
## 1. High — `doFlushHardware()` can dereference a null pointer (crash)
`src/turboPmacController.cpp:468-482`
```cpp
asynStatus turboPmacController::doFlushHardware() {
int temp = pTurboPmacC_->pasynInt32SyncIOipPort->reason; // <-- no null check
```
`pasynInt32SyncIOipPort` is a member, but it is only ever *declared* in `turboPmacController`; it is the **sinqController base constructor** that populates its own `pasynOctetSyncIOipPort`. The derived class declares its own `pasynInt32SyncIOipPort` (initialized to `nullptr`) and calls `pasynInt32SyncIO->connect(ipPortConfigName, 0, &pTurboPmacC_->pasynInt32SyncIOipPort, NULL)` at line 169 — but then checks the *stale* local `status` (line 171), which still holds the value from `callParamCallbacks()`, **not** the return value of `connect()`. So:
- If `connect()` fails, the IOC does **not** exit — it continues with `pasynInt32SyncIOipPort == nullptr`, and the first FLUSH_HARDWARE PV write segfaults.
- Even in the success case this is fragile: `connect()`'s real status is discarded.
Fix: `status = pasynInt32SyncIO->connect(...); if (status != asynSuccess || ...ptr == nullptr) exit(-1);` and add a null guard in `doFlushHardware()`.
## 2. High — same latent bug in the `sinqController` constructor
`sinqMotor/src/sinqController.cpp:154,167-175`
`asynStatus status = asynSuccess;` (line 154) is never assigned by `pasynOctetSyncIO->connect(...)` (line 167), so the check at line 169 (`status != asynSuccess || ptr == nullptr`) never fires on the status branch. A failed connect is only caught by the nullptr half — and only if `connect()` leaves the pointer null. The intended "exit IOC on fatal connect failure" is effectively broken.
## 3. High — `readResponse()` / `readOctet()` can consume up to **2× the full response**
`src/pmacAsynIPPort.cpp:523-575` and `748-831`
`inBuf_` is treated as if it were always empty before each `socketRead`:
```cpp
status = socketRead(pasynUser, inBuf_.data(), maxchars, &thisRead);
...
inBufTail_ = 0;
inBufHead_ = thisRead; // previous contents silently discarded
```
`readResponse()` is called from `readOctet()` **again** mid-loop (line 814) whenever `inBuf_` was drained, to fetch the rest of a long response. Each call does a fresh `recv()` and overwrites `inBuf_`, resetting head/tail. For a PMAC response longer than `INPUT_SIZE` (1493 bytes) split across several `recv()`s, the chunks read between `readResponse()` calls are **thrown away** — the client gets a response with missing middle data, and `writeRead()`'s carriage-return count check will then fail with a misleading "unexpected response" error. The original DLS code had the same structure, but there the asyn IP layer buffered between reads; here the socket is read directly, so data lost to the `recv()` window that was *not* read is simply gone.
Also note `readOctet()` calls `pmacReadReady()` (a full request/response exchange on the socket) **before** each refill (line 808-812), even though `readResponse()` does the exact same thing internally — doubling the round-trips and, worse, the `READREADY` reply bytes can interleave with the real response data.
## 4. High — `flushOctet()` leaves `inBuf_` holding just-read data, and returns it to the *next* reader
`src/pmacAsynIPPort.cpp:833-855`
`flushOctet()` discards bytes with `recv()` into a throwaway `cbuf`, but does **not** reset `inBufHead_`/`inBufTail_`. Any bytes already sitting in `inBuf_` (read by a prior `readResponse()` and not yet consumed by `readOctet()`) are *not* flushed — they will be returned by the next `readOctet()`. If the intent is "toss everything," the buffer must be reset too:
```cpp
inBufTail_ = 0;
inBufHead_ = 0;
```
(`pmacFlush()` does reset them at line 653-654, which shows the inconsistency.)
## 5. High — `socketRead()` can return success with `thisRead` still **uninitialized**
`src/pmacAsynIPPort.cpp:362-419`
`thisRead` is declared uninitialized (line 364). If `pollSocket()` returns `asynSuccess` but `pollstatus == 0` (poll timed out, no data), `recv()` is still called on a non-blocking socket and returns -1/EAGAIN — handled, fine. But if `pollstatus > 0` and `recv()` then returns -1 with *another* errno that is neither EWOULDBLOCK/EINTR (e.g. ECONNRESET after closeConnection sets aside), status becomes `asynError` and the early path at 401-402 sets it — OK. The real hole: after the `if (thisRead < 0) {...}` block, if `status == asynError` was set but the code falls through, `*nbytesRead = thisRead` with `thisRead = 0` (line 413 clamps) — that's actually fine. **However** if `pollSocket()` itself fails (returns `asynError`) the function returns early (line 388) — fine. The genuinely bad case: `pollstatus == 0` but `recv()` is *not* called... it always is. On re-inspection the uninitialized read is masked; downgrade this to a **style/robustness** note: initialize `thisRead = 0`.
## 6. Medium — `writeRead()` counts CRs in a buffer that is not NUL-terminated by `strlen` safely
`src/turboPmacController.cpp:381-385`
```cpp
for (size_t i = 0; i < strlen(response); i++)
if (response[i] == '\r') numReceivedResponses++;
```
`response` is only NUL-terminated up to the first NUL. `readOctet()` can return binary-ish PMAC data (STX/BELL/ACK bytes) and the "response" is counted by `\r`. If the PMAC response contains a NUL byte (possible in binary modes, and `Ixx28`/`Qxx00` can in principle), `strlen` truncates the count. More concretely: `pasynOctetSyncIO->writeRead()` with EOS=`\006` returns `nbytesIn` — use `nbytesIn` (and scan that many bytes) instead of `strlen()`.
## 7. Medium — format string: `size_t` printed with `%ld`
`sinqMotor/src/sinqController.cpp:566-571` and `575-578`
```cpp
"More than %ld communication timeouts in %ld seconds.%s\n",
... pSinqC_->maxNumberTimeouts, pSinqC_->comTimeoutWindow, ...
```
`maxNumberTimeouts` is `size_t` and `comTimeoutWindow` is `time_t` — `%ld` is only correct for `long`. On platforms where `size_t` is 32-bit (or `time_t` differs) this is UB. Use `%zu` / cast to `time_t` explicitly.
## 8. Medium — FFI wrappers don't `return` after a null `dynamic_cast`
`sinqMotor/src/sinqAxis.cpp:749-768` (`setWatchdogEnabled`), `795-815` (`setOffsetMovTimeout`), `843-864` (`setScaleMovTimeout`)
```cpp
if (axis == nullptr) {
errlogPrintf(...);
} // <-- no return here
return axis->setWatchdogEnabled(enable != 0); // NULL deref
```
`setWatchdogEnabled` and `setOffsetMovTimeout` log and then **fall through** to dereferencing `axis` when the cast fails (only `setScaleMovTimeout` at line 859-863 has the correct `return asynError;`). Add the missing `return asynError;`.
Also in `setOffsetMovTimeout` (line 830): the iocsh arg is registered as `iocshArgInt` (`offsetMovTimeoutArg2`) but the FFI takes a `double` and the caller passes `args[2].ival` into a `double` parameter — works by implicit conversion, but inconsistent with `setScaleMovTimeout` which uses `iocshArgDouble`.
## 9. Medium — `msgPrintControlKey` uses `char*` (non-const) as a public API type
`sinqMotor/src/msgPrintControl.h:28-29, 146`
```cpp
msgPrintControlKey(char *controller_, ...);
bool shouldBePrinted(char *controller, ...);
```
`controller`/`portName` are `const char*` everywhere in the EPICS API. Every caller is forced to either hold a non-const buffer or cast. This is a design wart, not a memory bug, but it will cause `-Werror` friction and invites `const_cast` abuse. Change to `const char*` (the member is a `std::string`).
## 10. Medium — `startMovTimeoutWatchdog()` computes the timeout from stale/zero fields
`sinqMotor/src/sinqAxis.cpp:595-674`
- `motorPosition(&motorPos)` is called at line 620, but `pl_status` is then *re-tested* at lines 644 and 656 as if it were still the position read — it is, but the pattern is confusing; more importantly, `targetPosition` is only set in `sinqAxis::move()`, not in `doMove()` overrides that set it themselves. If a derived `doMove` starts a move without going through `sinqAxis::move()`, `targetPosition` is 0 and the watchdog deadline is wrong.
- `timeAccel = 2 * std::ceil(motorVelocity / motorAccel)` (line 663) — `motorAccel` here is `motorVelocityRec / motorAccelRec` (line 660), i.e. a *time*, so `motorVelocity / motorAccel` is not a time; the formula `2*v/a` expects `a` in units/s². This looks dimensionally wrong and will under/over-estimate the accel phase. Worth checking against the intended physics.
## 11. Medium — `enable()` can return `asynSuccess` without the axis actually reaching the requested state
`src/turboPmacAxis.cpp:1186-1193`
```cpp
if ((axStatus != -3) == on) {
... "Axis is already %s." ...
return asynSuccess;
}
```
`axStatus` here is read from `pTurboPmacA_->axisStatus`, which is *only* updated by `doPoll()`/`init()`. If the axis was deactivated (−5) or in E‑stop (−4) after the last poll, `axStatus` is stale, so the "already enabled" fast path can misfire and skip the enable command. The `doPoll(&moving)` at line 1159 updates `pTurboPmacA_->axisStatus` only if the poll's read of `P%2.2d00` succeeds *and* is parsed — if `writeRead` fails inside that poll, `axisStatus` keeps its old value. Consider reading the status directly here instead of trusting the cached member.
## 12. Low — `connectIt()` sets `fd_` after `setsockopt`, leaking the fd on the failure path
`src/pmacAsynIPPort.cpp:289-301`
If `setsockopt(TCP_NODELAY)` fails, the socket is destroyed — fine. But between `connect()` success (line 249) and `fd_ = fd` (line 300), an exception/early return would leak. More usefully: `connect()` itself (line 315) sets `inBufTail_/inBufHead_ = 0` **after** a successful connect, but if the PMAC already has queued data from a previous session it is read into a fresh buffer — acceptable, but combined with #4 the flush-on-reconnect story is inconsistent.
## 13. Low — `pollSocket()` ignores the `pollmsec < 0` (infinite) EINTR time check
`src/pmacAsynIPPort.cpp:339-360`
```cpp
if (epicsTimeDiffInSeconds(&endTime, &startTime) * 1000. > pollmsec)
break;
```
When `pollmsec` is negative (infinite wait), the comparison `> pollmsec` is always true on the first EINTR, so the loop breaks immediately and treats an interrupted infinite poll as "done" (pollstatus stays -1). The caller then proceeds to `recv()`/`send()` on a possibly-not-ready socket. Guard with `if (pollmsec >= 0 && elapsed > pollmsec)`.
## 14. Low — dead / misleading members
- `turboPmacControllerImpl::lastResponse[MAXBUF_]` (`src/turboPmacController.cpp:48`) is written nowhere — dead.
- `asynStatus errorStatus` shadowing in `turboPmacAxis::doPoll` (line 577) is fine, but the variable `nvals`/`motorRecResolution` at line 307 in `doPoll` is read via `getAxisParamChecked` but never used afterward — unused (would trip `-Wunused-but-set-variable`-style scrutiny under `-Werror`; it currently compiles only because it's "used" by the macro call).
---
### Summary of the top fixes I'd do first
1. `turboPmacController` ctor: capture the real return of `pasynInt32SyncIO->connect()` and exit on failure; null-guard in `doFlushHardware()` (#1).
2. `sinqController` ctor: same fix for `pasynOctetSyncIO->connect()` (#2).
3. Reset `inBuf_` in `flushOctet()` and make `readResponse()`/`readOctet()` handle a partially-filled `inBuf_` instead of overwriting it (#3, #4).
4. Add the missing `return asynError;` in the two `sinqAxis` FFI wrappers (#8).
5. Scan `nbytesIn` bytes instead of `strlen(response)` in `writeRead()` (#6).
Want me to apply any of these fixes? I'd suggest starting with #1–#2 (crash safety) since they're small, self-contained, and unambiguously correct.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Review done by Qwen3.8 (code was written by another model)
I've finished a thorough review of the whole tree (the
ai_slop2branch: the C→C++pmacAsynIPPortrewrite, the sinqMotor submodule,turboPmacController/turboPmacAxis, db/Makefile, and utils). Here's what I found, most severe first.1. High —
doFlushHardware()can dereference a null pointer (crash)src/turboPmacController.cpp:468-482pasynInt32SyncIOipPortis a member, but it is only ever declared inturboPmacController; it is the sinqController base constructor that populates its ownpasynOctetSyncIOipPort. The derived class declares its ownpasynInt32SyncIOipPort(initialized tonullptr) and callspasynInt32SyncIO->connect(ipPortConfigName, 0, &pTurboPmacC_->pasynInt32SyncIOipPort, NULL)at line 169 — but then checks the stale localstatus(line 171), which still holds the value fromcallParamCallbacks(), not the return value ofconnect(). So:connect()fails, the IOC does not exit — it continues withpasynInt32SyncIOipPort == nullptr, and the first FLUSH_HARDWARE PV write segfaults.connect()'s real status is discarded.Fix:
status = pasynInt32SyncIO->connect(...); if (status != asynSuccess || ...ptr == nullptr) exit(-1);and add a null guard indoFlushHardware().2. High — same latent bug in the
sinqControllerconstructorsinqMotor/src/sinqController.cpp:154,167-175asynStatus status = asynSuccess;(line 154) is never assigned bypasynOctetSyncIO->connect(...)(line 167), so the check at line 169 (status != asynSuccess || ptr == nullptr) never fires on the status branch. A failed connect is only caught by the nullptr half — and only ifconnect()leaves the pointer null. The intended "exit IOC on fatal connect failure" is effectively broken.3. High —
readResponse()/readOctet()can consume up to 2× the full responsesrc/pmacAsynIPPort.cpp:523-575and748-831inBuf_is treated as if it were always empty before eachsocketRead:readResponse()is called fromreadOctet()again mid-loop (line 814) wheneverinBuf_was drained, to fetch the rest of a long response. Each call does a freshrecv()and overwritesinBuf_, resetting head/tail. For a PMAC response longer thanINPUT_SIZE(1493 bytes) split across severalrecv()s, the chunks read betweenreadResponse()calls are thrown away — the client gets a response with missing middle data, andwriteRead()'s carriage-return count check will then fail with a misleading "unexpected response" error. The original DLS code had the same structure, but there the asyn IP layer buffered between reads; here the socket is read directly, so data lost to therecv()window that was not read is simply gone.Also note
readOctet()callspmacReadReady()(a full request/response exchange on the socket) before each refill (line 808-812), even thoughreadResponse()does the exact same thing internally — doubling the round-trips and, worse, theREADREADYreply bytes can interleave with the real response data.4. High —
flushOctet()leavesinBuf_holding just-read data, and returns it to the next readersrc/pmacAsynIPPort.cpp:833-855flushOctet()discards bytes withrecv()into a throwawaycbuf, but does not resetinBufHead_/inBufTail_. Any bytes already sitting ininBuf_(read by a priorreadResponse()and not yet consumed byreadOctet()) are not flushed — they will be returned by the nextreadOctet(). If the intent is "toss everything," the buffer must be reset too:(
pmacFlush()does reset them at line 653-654, which shows the inconsistency.)5. High —
socketRead()can return success withthisReadstill uninitializedsrc/pmacAsynIPPort.cpp:362-419thisReadis declared uninitialized (line 364). IfpollSocket()returnsasynSuccessbutpollstatus == 0(poll timed out, no data),recv()is still called on a non-blocking socket and returns -1/EAGAIN — handled, fine. But ifpollstatus > 0andrecv()then returns -1 with another errno that is neither EWOULDBLOCK/EINTR (e.g. ECONNRESET after closeConnection sets aside), status becomesasynErrorand the early path at 401-402 sets it — OK. The real hole: after theif (thisRead < 0) {...}block, ifstatus == asynErrorwas set but the code falls through,*nbytesRead = thisReadwiththisRead = 0(line 413 clamps) — that's actually fine. However ifpollSocket()itself fails (returnsasynError) the function returns early (line 388) — fine. The genuinely bad case:pollstatus == 0butrecv()is not called... it always is. On re-inspection the uninitialized read is masked; downgrade this to a style/robustness note: initializethisRead = 0.6. Medium —
writeRead()counts CRs in a buffer that is not NUL-terminated bystrlensafelysrc/turboPmacController.cpp:381-385responseis only NUL-terminated up to the first NUL.readOctet()can return binary-ish PMAC data (STX/BELL/ACK bytes) and the "response" is counted by\r. If the PMAC response contains a NUL byte (possible in binary modes, andIxx28/Qxx00can in principle),strlentruncates the count. More concretely:pasynOctetSyncIO->writeRead()with EOS=\006returnsnbytesIn— usenbytesIn(and scan that many bytes) instead ofstrlen().7. Medium — format string:
size_tprinted with%ldsinqMotor/src/sinqController.cpp:566-571and575-578maxNumberTimeoutsissize_tandcomTimeoutWindowistime_t—%ldis only correct forlong. On platforms wheresize_tis 32-bit (ortime_tdiffers) this is UB. Use%zu/ cast totime_texplicitly.8. Medium — FFI wrappers don't
returnafter a nulldynamic_castsinqMotor/src/sinqAxis.cpp:749-768(setWatchdogEnabled),795-815(setOffsetMovTimeout),843-864(setScaleMovTimeout)setWatchdogEnabledandsetOffsetMovTimeoutlog and then fall through to dereferencingaxiswhen the cast fails (onlysetScaleMovTimeoutat line 859-863 has the correctreturn asynError;). Add the missingreturn asynError;.Also in
setOffsetMovTimeout(line 830): the iocsh arg is registered asiocshArgInt(offsetMovTimeoutArg2) but the FFI takes adoubleand the caller passesargs[2].ivalinto adoubleparameter — works by implicit conversion, but inconsistent withsetScaleMovTimeoutwhich usesiocshArgDouble.9. Medium —
msgPrintControlKeyuseschar*(non-const) as a public API typesinqMotor/src/msgPrintControl.h:28-29, 146controller/portNameareconst char*everywhere in the EPICS API. Every caller is forced to either hold a non-const buffer or cast. This is a design wart, not a memory bug, but it will cause-Werrorfriction and invitesconst_castabuse. Change toconst char*(the member is astd::string).10. Medium —
startMovTimeoutWatchdog()computes the timeout from stale/zero fieldssinqMotor/src/sinqAxis.cpp:595-674motorPosition(&motorPos)is called at line 620, butpl_statusis then re-tested at lines 644 and 656 as if it were still the position read — it is, but the pattern is confusing; more importantly,targetPositionis only set insinqAxis::move(), not indoMove()overrides that set it themselves. If a deriveddoMovestarts a move without going throughsinqAxis::move(),targetPositionis 0 and the watchdog deadline is wrong.timeAccel = 2 * std::ceil(motorVelocity / motorAccel)(line 663) —motorAccelhere ismotorVelocityRec / motorAccelRec(line 660), i.e. a time, somotorVelocity / motorAccelis not a time; the formula2*v/aexpectsain units/s². This looks dimensionally wrong and will under/over-estimate the accel phase. Worth checking against the intended physics.11. Medium —
enable()can returnasynSuccesswithout the axis actually reaching the requested statesrc/turboPmacAxis.cpp:1186-1193axStatushere is read frompTurboPmacA_->axisStatus, which is only updated bydoPoll()/init(). If the axis was deactivated (−5) or in E‑stop (−4) after the last poll,axStatusis stale, so the "already enabled" fast path can misfire and skip the enable command. ThedoPoll(&moving)at line 1159 updatespTurboPmacA_->axisStatusonly if the poll's read ofP%2.2d00succeeds and is parsed — ifwriteReadfails inside that poll,axisStatuskeeps its old value. Consider reading the status directly here instead of trusting the cached member.12. Low —
connectIt()setsfd_aftersetsockopt, leaking the fd on the failure pathsrc/pmacAsynIPPort.cpp:289-301If
setsockopt(TCP_NODELAY)fails, the socket is destroyed — fine. But betweenconnect()success (line 249) andfd_ = fd(line 300), an exception/early return would leak. More usefully:connect()itself (line 315) setsinBufTail_/inBufHead_ = 0after a successful connect, but if the PMAC already has queued data from a previous session it is read into a fresh buffer — acceptable, but combined with #4 the flush-on-reconnect story is inconsistent.13. Low —
pollSocket()ignores thepollmsec < 0(infinite) EINTR time checksrc/pmacAsynIPPort.cpp:339-360When
pollmsecis negative (infinite wait), the comparison> pollmsecis always true on the first EINTR, so the loop breaks immediately and treats an interrupted infinite poll as "done" (pollstatus stays -1). The caller then proceeds torecv()/send()on a possibly-not-ready socket. Guard withif (pollmsec >= 0 && elapsed > pollmsec).14. Low — dead / misleading members
turboPmacControllerImpl::lastResponse[MAXBUF_](src/turboPmacController.cpp:48) is written nowhere — dead.asynStatus errorStatusshadowing inturboPmacAxis::doPoll(line 577) is fine, but the variablenvals/motorRecResolutionat line 307 indoPollis read viagetAxisParamCheckedbut never used afterward — unused (would trip-Wunused-but-set-variable-style scrutiny under-Werror; it currently compiles only because it's "used" by the macro call).Summary of the top fixes I'd do first
turboPmacControllerctor: capture the real return ofpasynInt32SyncIO->connect()and exit on failure; null-guard indoFlushHardware()(#1).sinqControllerctor: same fix forpasynOctetSyncIO->connect()(#2).inBuf_influshOctet()and makereadResponse()/readOctet()handle a partially-filledinBuf_instead of overwriting it (#3, #4).return asynError;in the twosinqAxisFFI wrappers (#8).nbytesInbytes instead ofstrlen(response)inwriteRead()(#6).Want me to apply any of these fixes? I'd suggest starting with #1–#2 (crash safety) since they're small, self-contained, and unambiguously correct.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.