Fix bvlc_register_with_bbmd() reporting FAILED on real success by bcorrion · Pull Request #66 · IOTechSystems/bacnet-stack

bcorrion · 2026-08-21T20:05:57Z

Summary

dlenv_register_as_foreign_device() runs synchronously from main(), before the receive loop starts, so bvlc_register_with_bbmd()'s pthread_cond_timedwait() was blocking on a signal that can only ever come from bvlc_handler() processing a BVLC_RESULT -- and bvlc_handler() only runs as a side effect of bip_receive(), which is only ever called from the main receive loop that hasn't started yet. Nothing else reads the socket during the wait, so it could never observe the BBMD's real ACK -- only time out and report failure, even when registration genuinely succeeded on the wire.
Confirmed against a real BBMD (iotech-mini-bbmd): its own log showed the registration being received and processed correctly while bacnet-sim simultaneously self-reported FAILED to Register with BBMD.
Fix: poll bip_receive() directly during the wait instead of relying on a condition variable nothing can signal in time. bip_receive() already invokes bvlc_handler() internally as a side effect, so this correctly observes the real ACK/NAK.

Test plan

Built a real bacnet-sim binary from this patched stack via the project's own Docker build steps.
Ran it on a real Linux host against a live, running iotech-mini-bbmd.
bacnet-sim's own log now reads SUCCEEDED to Register with BBMD at <addr>:<port> (previously always printed FAILED, even on success).
mini-bbmd's own log independently confirmed Registering Foreign Device from address: <ip>:<port> matching the test container's actual IP exactly, at the same timestamp -- a genuine two-sided success, not just a self-report.

Worth checking whether device-bacnet-ip or other consumers of this shared stack call the same bvlc_register_with_bbmd() path before merging, since this fix could affect them too.

🤖 Generated with Claude Code

dlenv_register_as_foreign_device() runs synchronously from main(), before the receive loop starts, so bvlc_register_with_bbmd()'s pthread_cond_timedwait() was blocking on a signal that can only ever come from bvlc_handler() processing a BVLC_RESULT -- and bvlc_handler() only ever runs as a side effect of bip_receive(), which is only ever called from the main receive loop that hasn't started yet. Nothing else reads the socket during the wait (bacnet-sim's only other thread is the unrelated Lua script runner, started later), so the wait could never observe the BBMD's real ACK -- it could only time out and report failure, even when registration genuinely succeeded on the wire. Confirmed against a real BBMD (iotech-mini-bbmd): its own log showed the registration being received and processed correctly while bacnet-sim simultaneously self-reported "FAILED to Register with BBMD". Fix: poll bip_receive() directly during the wait instead of relying on a condition variable nothing can signal in time. bip_receive() already invokes bvlc_handler() internally as a side effect, so this correctly observes the real ACK/NAK. Verified with a rebuilt bacnet-sim binary against a live mini-bbmd: now logs "SUCCEEDED to Register with BBMD", with mini-bbmd's own log independently confirming the same registration event from the exact matching source address at the same timestamp. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot

Pull request overview

This PR addresses a startup-time BACnet/IP foreign-device registration issue where bvlc_register_with_bbmd() could time out and report failure even when the BBMD ACK/NAK was received on the wire, by actively polling bip_receive() during the registration wait. In addition, the PR bundles a large set of stack changes (new services/objects, API changes, platform portability tweaks, and versioning updates) that materially expand scope beyond the BBMD fix.

Changes:

Update BBMD foreign-device registration flow to pump bip_receive() while waiting for a BVLC_RESULT.
Introduce broad API and object-model changes (per-destination TSM API, “Direct” service send helpers, dynamic object storage, new object fields).
Add restart notification support plus assorted platform/build updates and version/changelog updates.

Reviewed changes

Copilot reviewed 112 out of 112 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
VERSION	Add repository version file (1.2.7).
CHANGELOG.md	Add release notes up through v1.2.7.
src/bacnet/version.h	Update BACnet version macros (noted mismatch in review).
src/bacnet/timesync.h	Refactor recipient list type into shared header.
src/bacnet/recipients.h	New shared `BACNET_RECIPIENT_LIST` definition.
src/bacnet/restart.h	New restart recipients encode/decode API.
src/bacnet/restart.c	Implement restart recipient list APDU encode/decode.
src/bacnet/npdu.c	Update test stub signature for `tsm_free_invoke_id`.
src/bacnet/event.h	Extend event notification parameters (command failure/access/timer/etc).
src/bacnet/datalink/dlenv.h	Change `dlenv_init()` to return `int`.
src/bacnet/datalink/dlenv.c	Adjust init flow and BBMD registration logging/behavior.
src/bacnet/cov.c	Extend COV notify decode (restart timestamp handling) and list iteration changes.
src/bacnet/config.h	Change vendor defaults and various sizing constants.
src/bacnet/basic/tsm/tsm.h	Change TSM API to be per-destination (`BACNET_ADDRESS *`).
src/bacnet/basic/services.h	Add restart service handler header include.
src/bacnet/basic/service/s_wpm.h	Add direct-address WPM send API.
src/bacnet/basic/service/s_wpm.c	Implement WPM direct send and adapt to per-dest TSM API.
src/bacnet/basic/service/s_wp.h	Add direct-address WP send APIs.
src/bacnet/basic/service/s_wp.c	Implement WP direct send variants and adapt to per-dest TSM API.
src/bacnet/basic/service/s_rpm.c	Adapt RPM client send to per-dest TSM API.
src/bacnet/basic/service/s_rp.c	Adapt RP send to per-dest TSM API.
src/bacnet/basic/service/s_readrange.c	Adapt ReadRange send to per-dest TSM API.
src/bacnet/basic/service/s_rd.c	Adapt ReinitializeDevice send to per-dest TSM API.
src/bacnet/basic/service/s_lso.c	Adapt LifeSafetyOperation send to per-dest TSM API.
src/bacnet/basic/service/s_getevent.c	Adapt GetEvent send to per-dest TSM API.
src/bacnet/basic/service/s_get_event.c	Adapt GetEventInformation send to per-dest TSM API.
src/bacnet/basic/service/s_get_alarm_sum.c	Adapt GetAlarmSummary send to per-dest TSM API.
src/bacnet/basic/service/s_dcc.c	Adapt DCC send to per-dest TSM API.
src/bacnet/basic/service/s_cov.h	Add direct-address SubscribeCOV / SubscribeCOVProperty APIs.
src/bacnet/basic/service/s_cov.c	Implement direct send for SubscribeCOV and SubscribeCOVProperty.
src/bacnet/basic/service/s_cevent.c	Adapt ConfirmedEventNotification send to per-dest TSM API.
src/bacnet/basic/service/s_awfs.c	Adapt AtomicWriteFile send to per-dest TSM API.
src/bacnet/basic/service/s_arfs.c	Adapt AtomicReadFile send to per-dest TSM API.
src/bacnet/basic/service/s_ack_alarm.c	Adapt AlarmAck send to per-dest TSM API.
src/bacnet/basic/service/h_rpm_a.c	RPM ACK decode adjustments for edge cases/empty values.
src/bacnet/basic/service/h_restart.h	New restart handler API.
src/bacnet/basic/service/h_restart.c	Implement restart recipient management + send logic.
src/bacnet/basic/service/h_cov.h	Add SubscribeCOVProperty handler and `handler_cov_all()`.
src/bacnet/basic/service/h_cov.c	Add `handler_cov_all()` and SubscribeCOVProperty handler path; adapt TSM calls.
src/bacnet/basic/service/h_apdu.c	Adapt invoke-id free calls to per-source address signature.
src/bacnet/basic/object/trendlog.h	Move `TL_MAX_ENTRIES` and add TREND_LOG_DESCR and new object mgmt APIs.
src/bacnet/basic/object/schedule.h	Add name/description fields and dynamic object mgmt APIs.
src/bacnet/basic/object/schedule.c	Convert schedules to dynamically allocated list with mutex.
src/bacnet/basic/object/piv.h	Widen PositiveInteger PV type and add metadata/object mgmt APIs.
src/bacnet/basic/object/osv.h	Add name/description and dynamic object mgmt APIs.
src/bacnet/basic/object/osv.c	Convert OSV objects to dynamically allocated list with mutex.
src/bacnet/basic/object/netport.h	Update description getter signature; add dynamic object mgmt APIs.
src/bacnet/basic/object/nc.h	Add name/description, dynamic mgmt APIs, and rename recipient finder.
src/bacnet/basic/object/nc.c	Convert NC objects to dynamic list with mutex; add register destination helper.
src/bacnet/basic/object/msv.h	Add multistate value descriptor and dynamic mgmt APIs.
src/bacnet/basic/object/mso.h	Add MSO descriptor, description getter signature change, dynamic mgmt APIs.
src/bacnet/basic/object/mso.c	Convert MSO storage to dynamic descriptor with mutex.
src/bacnet/basic/object/ms-input.h	Add multistate input descriptor and dynamic mgmt APIs.
src/bacnet/basic/object/lsp.h	Add LSP descriptor, name/description, dynamic mgmt APIs.
src/bacnet/basic/object/lsp.c	Convert LSP storage to dynamic descriptor with mutex.
src/bacnet/basic/object/lo.h	Update description getter signature; add dynamic mgmt APIs.
src/bacnet/basic/object/lc.h	Add Load Control descriptor, metadata fields, and dynamic mgmt APIs.
src/bacnet/basic/object/iv.h	Add integer descriptor, metadata, and dynamic mgmt APIs.
src/bacnet/basic/object/device.h	Add object cleanup hook and vendor-name setter API.
src/bacnet/basic/object/csv.h	Add CSV descriptor and dynamic mgmt APIs.
src/bacnet/basic/object/csv.c	Convert CSV storage to dynamic descriptor with mutex.
src/bacnet/basic/object/command.h	Update description getter signature; add metadata and dynamic mgmt APIs.
src/bacnet/basic/object/command.c	Convert Command objects to dynamic list with mutex; add name/description helpers.
src/bacnet/basic/object/client/device-client.c	Add `Device_getCurrentTime()` implementation.
src/bacnet/basic/object/channel.h	Add Channel description getters/setters and dynamic mgmt APIs.
src/bacnet/basic/object/bv.h	Add BV descriptor, set-properties, priority PV set, and dynamic mgmt APIs.
src/bacnet/basic/object/bo.h	Add BO descriptor, set-properties, description getter signature, dynamic mgmt APIs.
src/bacnet/basic/object/bi.h	Add BI descriptor, set-properties, description getter signature, dynamic mgmt APIs.
src/bacnet/basic/object/bacfile.h	Add bacfile dynamic mgmt APIs and cleanup hook.
src/bacnet/basic/object/bacfile.c	Convert bacfile listing to dynamic allocation and add init/cleanup helpers.
src/bacnet/basic/object/av.h	Add AV metadata fields, set-properties, and dynamic mgmt APIs.
src/bacnet/basic/object/ao.h	Add AO descriptor, set-properties, metadata, and dynamic mgmt APIs.
src/bacnet/basic/object/ai.h	Add AI metadata fields, set-properties, and dynamic mgmt APIs.
src/bacnet/basic/object/acc.h	Add accumulator descriptor, set-properties, metadata, and dynamic mgmt APIs.
src/bacnet/basic/object/acc.c	Convert Accumulator storage to dynamic descriptor with mutex; add name/desc.
src/bacnet/basic/bbmd/h_bbmd.c	Implement BBMD registration wait polling and related sync state.
src/bacnet/bacenum.h	Add timer state/transition enums.
src/bacnet/bacapp.h	Add `BACNET_TIMESTAMP` to app data union and include timestamp header.
src/bacnet/bacapp.c	Add timestamp encoding and “complex” context-data skipping logic.
ports/linux/rs485.c	Initialize `struct timeval` to avoid uninitialized use.
ports/linux/dlmstp.c	Add thread lifecycle controls and ring buffer mutex protection.
ports/linux/dlmstp_linux.h	Replace pthread internal header with `sys/types.h`.
ports/linux/datetime-init.c	Guard `timezone` usage for Azure Sphere.
ports/linux/bip-init.c	Add Azure Sphere interface/address discovery path; tweak error return handling.
ports/linux/bacport.h	Guard Linux-only headers for Azure Sphere.
ports/esp32/src/device.h	Add `Device_getCurrentTime()` prototype.
ports/esp32/src/device.c	Add `Device_getCurrentTime()` implementation.
ports/esp32/src/bo.h	Update Binary Output description getter signature.
apps/server/Makefile	Add Lua/pthread link flags and simulator defines.
apps/Makefile	Add simulator defines and increase MAX_COV_SUBCRIPTIONS define.
apps/scov/main.c	Add high-volume subscription send loop (but noted TSM API mismatch in review).
apps/piface/device.c	Add `Device_getCurrentTime()` implementation.

Suppressed comments (2)

src/bacnet/basic/bbmd/h_bbmd.c:1211

pthread_cond_init(&cond, ...) is never paired with pthread_cond_destroy(&cond), and the mutex is destroyed while the BVLC_RESULT handler can still lock/signal using these globals later. This can lead to undefined behavior after bvlc_register_with_bbmd() returns. Prefer keeping the mutex/condvar initialized for process lifetime (static initializers) or add a clear lifecycle/guard so the handler never touches destroyed objects.

    pthread_mutex_destroy(&mutex);

    /* Fail if the BBMD registration was not successful */
    if (bbmd_reg != BBMD_REG_SUCCESS)
    {
      retval = -1;
    }

src/bacnet/bacapp.c:1729

apdu_len++ /* jump closing tag */ executes unconditionally, even if the loop terminated because apdu_len >= max_apdu_len (i.e., no closing tag was found). This can advance beyond the available buffer and miscompute consumed length. Return an error when the closing tag is not present within max_apdu_len instead of incrementing unconditionally.

    }
    apdu_len++; /* jump closing tag */

    return apdu_len;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

bcorrion · 2026-08-21T20:33:34Z

+    pthread_mutex_init (&mutex, NULL);
+
+    pthread_mutex_lock (&mutex);
+    /* Set the initial value of the BBMD registration bool to false */
+    bbmd_reg = BBMD_REG_UNSET;
+    pthread_mutex_unlock (&mutex);
+
+    /* bvlc_handler() still calls pthread_cond_signal(&cond) on receipt of
+     * a BVLC_RESULT (see below) -- kept initialized even though nothing
+     * waits on it anymore below, since a signal on an uninitialized
+     * condvar would be undefined behavior. */
+    pthread_cond_init (&cond, NULL);
+
+    int retval = bip_send_mpdu(bbmd_addr, &BVLC_Buffer[0], BVLC_Buffer_Len);
+    if (retval == -1)
+    {
+        return retval;
+    }


Fixed in ca0beae: switched mutex/cond to static initializers (PTHREAD_MUTEX_INITIALIZER/PTHREAD_COND_INITIALIZER) instead of runtime init/destroy. This resolves the leak on this early-return path directly -- there's no longer anything to destroy, since the objects are valid for the process lifetime.

bcorrion · 2026-08-21T20:33:41Z

+/** Mutex and condition variable for checking if BBMD registration has been successful */
+static pthread_mutex_t mutex;
+static pthread_cond_t cond;
+static bbmd_reg_t bbmd_reg;


Fixed in ca0beae: mutex/cond are now statically initialized (PTHREAD_MUTEX_INITIALIZER/PTHREAD_COND_INITIALIZER) at declaration instead of pthread_*_init() at runtime, so they're always valid from process start regardless of whether bvlc_register_with_bbmd() has run yet.

bcorrion · 2026-08-21T20:34:21Z

+        if(can_send_subs)
+        {
+            while(tsm_transaction_available() && subs < desired_subs)
+            {
+                cov_data->monitoredObjectIdentifier.instance = subs;
+                Request_Invoke_ID = Send_COV_Subscribe(Target_Device_Object_Instance, cov_data);
+                if(Request_Invoke_ID != 0)


This comment came from comparing against the wrong base branch (this PR was originally opened against master, which has diverged 173 commits from the v1.2.8-iotech tag this fix is actually built on). Retargeted the PR to v1.2-iotech-branch -- the diff is now scoped to just h_bbmd.c, and this file isn't part of it. Not something this PR touches or introduces.

bcorrion · 2026-08-21T20:34:22Z

    /* === Initialize the Datalink Here === */
    if (!datalink_init(getenv("BACNET_IFACE"))) {
-        exit(1);
-    }
-#if (MAX_TSM_TRANSACTIONS)
-    pEnv = getenv("BACNET_INVOKE_ID");
-    if (pEnv) {
-        tsm_invokeID_set((uint8_t)strtol(pEnv, NULL, 0));
+        return 1;
    }
-#endif
    dlenv_network_port_init();
-    dlenv_register_as_foreign_device();
+
+    return 0;


This comment came from comparing against the wrong base branch (this PR was originally opened against master, which has diverged 173 commits from the v1.2.8-iotech tag this fix is actually built on). Retargeted the PR to v1.2-iotech-branch -- the diff is now scoped to just h_bbmd.c, and this file isn't part of it. Not something this PR touches or introduces.

bcorrion · 2026-08-21T20:34:24Z

            /* a tag number of 2 is not extended so only one octet */
            len++;
            app_data = &value->value;
-            while (!decode_is_closing_tag_number(&apdu[len], 2)) {
+            bool more_values = !decode_is_closing_tag_number(&apdu[len], 2);
+            while (more_values) {
+                BACNET_APPLICATION_DATA_VALUE *next;


This comment came from comparing against the wrong base branch (this PR was originally opened against master, which has diverged 173 commits from the v1.2.8-iotech tag this fix is actually built on). Retargeted the PR to v1.2-iotech-branch -- the diff is now scoped to just h_bbmd.c, and this file isn't part of it. Not something this PR touches or introduces.

bcorrion · 2026-08-21T20:34:25Z

+                if (value->propertyIdentifier == PROP_TIME_OF_DEVICE_RESTART) {
+                    BACNET_TIMESTAMP stamp;
+                    app_len = bacapp_decode_timestamp(&apdu[len], &stamp);
+                    if (app_len < 0) {
+                        return BACNET_STATUS_ERROR;
+                    }
+                    app_data->next = NULL;
+                    app_data->context_specific = true;
+                    app_data->context_tag = stamp.tag;
+                    switch (stamp.tag) {
+                        case TIME_STAMP_TIME:
+                            app_data->tag = BACNET_APPLICATION_TAG_TIME;
+                            app_data->type.Time = stamp.value.time;
+                            break;
+                        case TIME_STAMP_SEQUENCE:
+                            app_data->tag = BACNET_APPLICATION_TAG_UNSIGNED_INT;
+                            app_data->type.Unsigned_Int = stamp.value.sequenceNum;
+                            break;
+                        case TIME_STAMP_DATETIME:
+                            app_data->tag = BACNET_APPLICATION_TAG_DATE;
+                            app_data->type.Date = stamp.value.dateTime.date;
+                            // TODO: decode stamp.value.dateTime.time into app_data->next
+                            break;
+                    }


This comment came from comparing against the wrong base branch (this PR was originally opened against master, which has diverged 173 commits from the v1.2.8-iotech tag this fix is actually built on). Retargeted the PR to v1.2-iotech-branch -- the diff is now scoped to just h_bbmd.c, and this file isn't part of it. Not something this PR touches or introduces.

bcorrion · 2026-08-21T20:34:26Z

+    /* If it's closed : leave */
+    while (!decode_is_closing_tag_number(&apdu[apdu_len], tag_number) &&
+           (apdu_len < max_apdu_len)) {
+        /* Context ou pas ! */


This comment came from comparing against the wrong base branch (this PR was originally opened against master, which has diverged 173 commits from the v1.2.8-iotech tag this fix is actually built on). Retargeted the PR to v1.2-iotech-branch -- the diff is now scoped to just h_bbmd.c, and this file isn't part of it. Not something this PR touches or introduces.

bcorrion · 2026-08-21T20:34:27Z

+#define BACNET_VERSION_TEXT "1.2.5"
+#define BACNET_VERSION_CODE BACNET_VERSION(1,2,5)


This comment came from comparing against the wrong base branch (this PR was originally opened against master, which has diverged 173 commits from the v1.2.8-iotech tag this fix is actually built on). Retargeted the PR to v1.2-iotech-branch -- the diff is now scoped to just h_bbmd.c, and this file isn't part of it. Not something this PR touches or introduces.

bvlc_handler() locks/signals mutex/cond unconditionally on any incoming BVLC_RESULT, including one that arrives before bvlc_register_with_bbmd() is ever called for the first time -- using them before pthread_mutex_init()/pthread_cond_init() ran would be undefined behavior. Separately, the early return on bip_send_mpdu() failure skipped pthread_mutex_destroy(), leaking the mutex on that path. Switch mutex/cond to static initializers (PTHREAD_MUTEX_INITIALIZER/ PTHREAD_COND_INITIALIZER) instead of runtime init/destroy -- they're now always valid for the life of the process, which resolves both issues at once and matches how BVLC_RESULT's handler already treats them as always-available globals. Re-verified live against production mini-bbmd after this change: bacnet-sim still logs "SUCCEEDED to Register with BBMD", with mini-bbmd's own log independently confirming the same registration event. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

bcorrion requested review from iain-anderson and a lite review from Copilot August 21, 2026 20:07

Copilot started reviewing on behalf of bcorrion August 21, 2026 20:07 View session

Copilot AI reviewed Aug 21, 2026

View reviewed changes

bcorrion changed the base branch from master to v1.2-iotech-branch August 21, 2026 20:24

Navigation Menu

Fix bvlc_register_with_bbmd() reporting FAILED on real success - #66

Fix bvlc_register_with_bbmd() reporting FAILED on real success#66
bcorrion wants to merge 2 commits into
v1.2-iotech-branchfrom
fix/bbmd-registration-ack-deadlock

bcorrion commented Aug 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

bcorrion Aug 21, 2026

Uh oh!

bcorrion Aug 21, 2026

Uh oh!

bcorrion Aug 21, 2026

Uh oh!

bcorrion Aug 21, 2026

Uh oh!

bcorrion Aug 21, 2026

Uh oh!

bcorrion Aug 21, 2026

Uh oh!

bcorrion Aug 21, 2026

Uh oh!

bcorrion Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		#define BACNET_VERSION_TEXT "1.2.5"
		#define BACNET_VERSION_CODE BACNET_VERSION(1,2,5)

Conversation

bcorrion commented Aug 21, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants