fix: fall back to per-index object-list scan when ReadProperty(OBJECT_LIST, ALL) fails

[ravz]

Fixes #53. Fixes #52. Addresses the report in #49.

The bug (#53)

A device that answers ReadProperty(OBJECT_NAME) but silently drops
ReadProperty(OBJECT_LIST, ALL) is never discovered by the scheduled poll. Its point list stays at
just the device object and the console fills with ERR_TIMEOUT, forever.

Devices behave this way when the full object-list response won't fit an unsegmented APDU — they drop
the request rather than returning an Error or Abort, so there is no error PDU, just a timeout. YABE
reads the same devices fine because it walks the object list by array index.

edge-bacnet already had that per-index scan (scanDeviceManually), but the scheduled loop could
never reach it: the fallback lived only in updatePointsForDevice, which fires solely from the read
node's right-click Update points menu. The only automatic route in was getSegmentation() == 3, so
a device that advertises segmentation support but can't actually deliver the list never qualified.

Confirmed the request itself is emitted correctly — this was never a send-side problem. Instrumenting
the bundled stack at the transport and dgram boundaries:

plain IP       / OBJECT_NAME  -> sent, 17 bytes   810a0011 0104 0275 00 0c 0c02001f40 19 4d
plain IP       / OBJECT_LIST  -> sent, 17 bytes   810a0011 0104 0275 01 0c 0c02001f40 19 4c
MSTP (net+adr) / both         -> sent, 22 bytes   (DNET/DADR encoded)

Changes

discoverPointList(device) — one strategy, shared by the scheduled loop and
updatePointsForDevice. Tries the single round-trip ALL read; on failure falls back to the
per-index scan. A successful fallback latches manualDiscoveryMode, so later polls skip the doomed
ALL read instead of burning an apduTimeout every cycle.

Success is judged by what the scan returned on this pass, not by growth of the device's cumulative
point list. That distinction matters: a latched device re-scanned in steady state finds the same
objects, setPointsList dedups them, and a count-delta test would report failure on every poll —
reinstating the console spam this PR is meant to remove.

scanDevice no longer calls setManualDiscoveryMode(false) as a side effect, which would otherwise
clobber the latch on the next poll.

queryDevices recursion → for loop (#52). A missing return in the getProtocolSupported
error path made each failing device traverse the remaining list twice — m failing devices produced
2^m traversals, each costing an apduTimeout per unresponsive device. pollInProgress now resets in
a finally; previously any exception escaping the loop left it stuck true, and since the scheduled
task is gated on !pollInProgress, that permanently disabled polling until a Node-RED restart.

Error logs now use getDeviceAddress(device) instead of interpolating the address object, which
rendered as [object Object] for MSTP devices — visible in the #49 report.

Verification

Driven through a stubbed client (every BACnet call in this file goes through
client.readProperty / client.readPropertyMultiple, so no network is needed):

pass 1: latched, points = 3
pass 2: no ALL read, no error logs, discoverPointList -> true
pass 3: no ALL read, no error logs, discoverPointList -> true
total failure: false, no latch, ALL-read cause logged
total failure: queryDevices logs the failure naming the device
healthy device: one ALL read, never latched

Covering: fallback recovers the point list and latches; subsequent polls issue no ALL read and log
no error; a device answering ALL normally still takes the one-shot path and never latches; total
failure returns false without latching and is logged with the device named.

Known limitations

scanDeviceManually resolves — rather than rejects — when a per-index read errors, treating any error
as end-of-array. So when the index scan yields nothing, discoverPointList has no underlying error to
log; it returns false and both call sites log the failure with the device named. That resolve-on-error
behaviour also means a transient timeout mid-scan silently truncates the point list. Tracked
separately in #50 and deliberately out of scope here.

#51 (transport silently drops sends to ports outside the configured Port Range while still arming the
APDU timeout) is also untouched — real, but unrelated to #49.

Testing notes

This repo has no test harness, so the verification above is a throwaway script rather than a committed
test. Worth standing one up; the stubbed-client approach works without a network and would cover this
area cheaply.

https://claude.ai/code/session_015Yf3sg2PeMKH29s3owfyxM

[Copilot]

Pull request overview

This PR fixes device point discovery when ReadProperty(OBJECT_LIST, ALL) times out by introducing a shared discovery strategy that falls back to a per-index object-list scan, and it also refactors the scheduled polling loop to eliminate exponential traversal and ensure pollInProgress always resets.

Changes:

• Add discoverPointList(device) to try OBJECT_LIST, ALL first and fall back to per-index scanning, latching manualDiscoveryMode after a successful fallback.
• Refactor queryDevices from recursive traversal to a for loop with try/finally to reliably clear pollInProgress and avoid repeated traversals on error.
• Update updatePointsForDevice to use the shared discovery helper instead of duplicating fallback logic.

Suppressed comments (2)

bacnet_client.js:1216

• Because getDevicePointList is now only used by discoverPointList, having it also log failures causes duplicate/noisy logs. It also logs the address via device.getAddress().toString(), which renders as [object Object] for MSTP devices. Prefer letting discoverPointList be the single place that logs point-list discovery failures, and have getDevicePointList just reject.

      try {
        let result = await that.scanDevice(device);
        device.setPointsList(result);
        device.setLastSeen(Date.now());

bacnet_client.js:645

• This log message is now misleading: the code no longer calls getDevicePointList here; it calls discoverPointList, which may use either the ALL read or the per-index scan. Renaming the log string makes failures easier to understand when triaging logs.

          const discoverySucceeded = await that.discoverPointList(device);
          if (!discoverySucceeded) {
            that.logOut(`getDevicePointList error: ${that.getDeviceAddress(device)} - ${device.getDeviceId()}`);
          }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ravz]

Both Copilot points addressed in 49a4879.

Duplicate logging / [object Object] — scanDevice and getDevicePointList each logged the same failure on the way out, so one timeout produced three lines, and getDevicePointList interpolated device.getAddress().toString(), which renders as [object Object] for MSTP devices. Both now just reject. discoverPointList is the single reporter, and it's the only frame that knows whether a fallback follows — so it can say "falling back" rather than "error" when a retry is coming. A total failure now logs two lines (cause + verdict) instead of four.

Misleading log string — getDevicePointList error: renamed to Point list discovery failed (both object-list strategies):. The old string named a function the call site no longer calls.

While in scanDevice I also changed the malformed-acknowledgement branch to reject rather than log-and-return. It was leaving the promise unsettled, which hangs the caller instead of surfacing the bad response.

---

Context worth flagging for whoever merges this: the reporter on #49 has since confirmed this does not fix their problem. It reproduces on a two-point network and the per-index fallback times out too, so the object-list read is failing for a reason unrelated to response size. This PR still fixes #53 and #52 on their own merits — the scheduled poll genuinely cannot reach the fallback, and the recursion bug is real — but it should not be expected to close #49.

Two further defects found while re-examining their new evidence, both filed separately with verification: #55 (per-property reads encode the raw max-APDU octet count into a 4-bit enum field, so a 480-octet device is told we accept 50, and a 206-octet device gets a reserved value) and #56 (_processError swallows decode failures, so a device error PDU surfaces as ERR_TIMEOUT). #55 is the likely cause of the no OBJECT_NAME returned (read likely rejected) drops in their log.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

bacnet_client.js:637

• In queryDevices, a getProtocolSupported failure currently continues the loop, which skips updateDeviceName and discoverPointList for that device. Point-list discovery uses ReadProperty(OBJECT_NAME/OBJECT_LIST) and does not depend on protocol-services-supported, so devices that error on PROTOCOL_SERVICES_SUPPORTED could be permanently skipped by the scheduled poll.

          } catch (error) {
            that.logOut("getProtocolSupported error: ", error);
            continue;
          }