You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Add non-blocking BBMD registration API; drop the self-poll fallback from #66 - #67
Follow-up to #66. That PR's fallback -- polling bip_receive() directly when the normal pthread_cond_timedwait() signal never arrives -- fixed bacnet-sim's real deadlock, but Copilot correctly flagged two problems with it after merge: it can race an already-running receive thread for datagrams on the same socket, and any ordinary NPDU it wins that race for is silently dropped instead of reaching the caller's normal dispatch path (bip_receive() only invokes bvlc_handler(), never the caller's npdu_handler()).
Rather than further narrow that race, this removes it entirely and gives the one caller that actually needs non-blocking behavior (bacnet-sim, single-threaded, calling this synchronously from main() before it has any receive loop running) a real non-blocking API instead, which needs no fallback because it never blocks in the first place.
bvlc_register_with_bbmd() reverts to a plain bounded pthread_cond_timedwait() -- unchanged in behavior from before Fix bvlc_register_with_bbmd() reporting FAILED on real success #66's self-poll fallback existed (the mutex/cond static-initializer fix from Fix bvlc_register_with_bbmd() reporting FAILED on real success #66 is kept; that was a genuine, independent lifecycle bug). This is correct for any caller with its own receive loop already running before it registers -- confirmed directly against device-bacnet-c-private's receive_data(), which starts its thread before ever calling register_as_foreign_device() -- and is the only remaining consumer of this function.
New bvlc_register_with_bbmd_async() + bvlc_bbmd_registration_status(): sends the request and returns immediately; the caller starts its own receive loop right after and polls the status once it's running. Never itself reads the socket, so it can never compete with anything else that does.
New dlenv_register_as_foreign_device_async() and dlenv_maintenance_timer_async() mirror the existing blocking pair for a caller using the async API throughout -- covering both the initial registration and periodic TTL renewal, so a single-threaded caller doesn't hit the same blocking wait on renewal that it avoided at startup.
Test plan
Built a real bacnet-sim binary from this patched stack via the project's own Docker build steps.
With BACNET_BBMD_TIMETOLIVE forced to 5s, four consecutive renewal cycles over 15s all succeeded -- independently confirmed by mini-bbmd's own log showing four separate RegisterForeignDevice events 5 seconds apart, matching bacnet-sim's four SUCCEEDED logs exactly.
bvlc_register_with_bbmd()'s self-poll fallback (added to fix bacnet-sim's
registration deadlock) had two real problems Copilot correctly flagged:
it can race an already-running receive thread for datagrams on the same
socket, and any ordinary NPDU it wins that race for is silently dropped
instead of reaching the caller's normal dispatch path. Rather than try
to further narrow that race, remove it: bacnet-sim (the only known
single-threaded caller) is moving to a new non-blocking API instead,
which needs no such fallback because it never blocks in the first place.
- bvlc_register_with_bbmd() reverts to a plain bounded
pthread_cond_timedwait(), unchanged in behavior from before the
self-poll fallback existed. This is correct for any caller with its
own receive loop already running before it registers -- confirmed
against device-bacnet-c-private's receive_data(), which does exactly
that -- and is the only remaining consumer of this function.
- New bvlc_register_with_bbmd_async() + bvlc_bbmd_registration_status():
sends the request and returns immediately; the caller starts its own
receive loop right after and polls the status once it's running. Never
itself reads the socket, so it can never compete with anything else
that does.
- New dlenv_register_as_foreign_device_async() and
dlenv_maintenance_timer_async() mirror the existing blocking pair for
a caller using the async API throughout -- covering both the initial
registration and periodic TTL renewal, so a single-threaded caller
doesn't hit the same blocking wait on renewal that it avoided at
startup.
Verified against a real running mini-bbmd with a bacnet-sim built from
this patched stack: initial registration logs "SUCCEEDED to Register
with BBMD" (previously "FAILED" before any of these fixes existed), and
with BACNET_BBMD_TIMETOLIVE forced to 5s, four consecutive renewal
cycles over 15s each succeeded -- independently confirmed by mini-bbmd's
own log showing four separate RegisterForeignDevice events 5 seconds
apart, matching bacnet-sim's four "SUCCEEDED" logs exactly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- dlenv_register_as_foreign_device_async() was entirely inside
#if defined(BACDL_BIP), so it didn't exist at all for BACDL_MSTP/
BACDL_BIP6 builds despite being declared unconditionally in the
header -- an undefined reference for any non-BIP consumer. Match
dlenv_register_as_foreign_device()'s existing pattern: guard the body,
not the signature/return, so it's a real no-op (returns 0) outside
BACDL_BIP instead of not existing.
- dlenv_maintenance_timer_async() always returned true once the TTL
countdown hit zero, regardless of whether
dlenv_register_as_foreign_device_async() actually sent anything (it
returns 0 if unconfigured, -1 if the send failed). A caller polling
bvlc_bbmd_registration_status() on a false positive would wait
forever for a status that will never change. Return type is now int,
matching dlenv_register_as_foreign_device_async()'s own contract
exactly, and callers (bacnet-sim's main.c) updated to match: a
negative return reports failure immediately (nothing to poll for),
same as a hard failure at startup.
- Reworded a stale comment in h_bbmd.h referring to
bvlc_register_with_bbmd()'s "self-polling fallback" -- that code path
was removed by this same PR, so the comment described something that
no longer exists in the function it was describing.
Verified against a real running mini-bbmd: initial registration still
logs "SUCCEEDED to Register with BBMD", and with
BACNET_BBMD_TIMETOLIVE forced to 5s, four consecutive renewal cycles
over 15s still succeeded, cross-confirmed by mini-bbmd's own log
showing four separate RegisterForeignDevice events 5 seconds apart.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Correlate BBMD registration results with the pending request
src/bacnet/basic/bbmd/h_bbmd.c:1217
This new status API is not scoped to an async registration request: the BVLC_RESULT handler updates bbmd_reg for every result, including results received before any registration call. A late reply from a previous renewal or an unrelated BBMD operation can therefore make this report SUCCEEDED/FAILED for the current request, after which the real reply is ignored because the handler only writes while the state is UNSET. Track a pending registration/generation and correlate the reply before publishing this status.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #66. That PR's fallback -- polling
bip_receive()directly when the normalpthread_cond_timedwait()signal never arrives -- fixed bacnet-sim's real deadlock, but Copilot correctly flagged two problems with it after merge: it can race an already-running receive thread for datagrams on the same socket, and any ordinary NPDU it wins that race for is silently dropped instead of reaching the caller's normal dispatch path (bip_receive()only invokesbvlc_handler(), never the caller'snpdu_handler()).Rather than further narrow that race, this removes it entirely and gives the one caller that actually needs non-blocking behavior (bacnet-sim, single-threaded, calling this synchronously from
main()before it has any receive loop running) a real non-blocking API instead, which needs no fallback because it never blocks in the first place.bvlc_register_with_bbmd()reverts to a plain boundedpthread_cond_timedwait()-- unchanged in behavior from before Fix bvlc_register_with_bbmd() reporting FAILED on real success #66's self-poll fallback existed (the mutex/cond static-initializer fix from Fix bvlc_register_with_bbmd() reporting FAILED on real success #66 is kept; that was a genuine, independent lifecycle bug). This is correct for any caller with its own receive loop already running before it registers -- confirmed directly againstdevice-bacnet-c-private'sreceive_data(), which starts its thread before ever callingregister_as_foreign_device()-- and is the only remaining consumer of this function.bvlc_register_with_bbmd_async()+bvlc_bbmd_registration_status(): sends the request and returns immediately; the caller starts its own receive loop right after and polls the status once it's running. Never itself reads the socket, so it can never compete with anything else that does.dlenv_register_as_foreign_device_async()anddlenv_maintenance_timer_async()mirror the existing blocking pair for a caller using the async API throughout -- covering both the initial registration and periodic TTL renewal, so a single-threaded caller doesn't hit the same blocking wait on renewal that it avoided at startup.Test plan
bacnet-simbinary from this patched stack via the project's own Docker build steps.mini-bbmd: initial registration logsSUCCEEDED to Register with BBMD(previouslyFAILEDbefore Fix bvlc_register_with_bbmd() reporting FAILED on real success #66 or this fix existed).BACNET_BBMD_TIMETOLIVEforced to 5s, four consecutive renewal cycles over 15s all succeeded -- independently confirmed by mini-bbmd's own log showing four separateRegisterForeignDeviceevents 5 seconds apart, matching bacnet-sim's fourSUCCEEDEDlogs exactly.device-bacnet-c-private's registration path (bvlc_register_with_bbmd, now simplified back to its pre-Fix bvlc_register_with_bbmd() reporting FAILED on real success #66 form plus the lifecycle fix) is unaffected -- verified itsreceive_data()thread starts before it ever registers, so the plain cond-wait signals correctly, same as before Fix bvlc_register_with_bbmd() reporting FAILED on real success #66.🤖 Generated with Claude Code