Skip to content

Conversation

valeriyvan
Copy link

@valeriyvan valeriyvan commented Aug 17, 2025

No description provided.

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
⚡ Recommended focus areas for review

Possible Logic Bug

The condition if (enabled && !(state.registers.vm0 | OSD_ENABLE)) uses bitwise OR inside a negation; this likely intends to check if the OSD_ENABLE bit is not set. Using OR here makes the expression always true when OSD_ENABLE is nonzero. Consider ((state.registers.vm0 & OSD_ENABLE) == 0) instead.

if (enabled && !(state.registers.vm0 | OSD_ENABLE)) {
    state.registers.vm0 |= OSD_ENABLE;
Busy-wait Risk

The loop while (!initialized) { registerDref(...); } may spin continuously and repeatedly register DREFs until initialized becomes true, potentially causing high CPU usage or duplicated registrations. Consider adding a sleep/backoff and/or move registration outside the loop or guard to run once.

while (!initialized) {
    registerDref(DREF_LATITUDE, "sim/flightmodel/position/latitude", 100);
    registerDref(DREF_LONGITUDE, "sim/flightmodel/position/longitude", 100);
    registerDref(DREF_ELEVATION, "sim/flightmodel/position/elevation", 100);
    registerDref(DREF_AGL, "sim/flightmodel/position/y_agl", 100);

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add timeout to initialization wait

Busy-waiting on initialized without a timeout can hang forever if the thread
fails or network is unreachable. Add a bounded wait with a timeout and small
sleep/yield to avoid spinning. Return failure if initialization does not
complete in time.

src/main/target/SITL/sim/xplane.c [483-487]

 static bool initialized = false;
 ...
 if (pthread_create(&listenThread, NULL, listenWorker, NULL) < 0) {
     return false;
 }
 
-while (!initialized) {
+// Wait for initialization with timeout to avoid indefinite hang
+const uint32_t initTimeoutMs = 5000; // 5s
+const uint32_t pollIntervalMs = 10;
+uint32_t waitedMs = 0;
+while (!initialized && waitedMs < initTimeoutMs) {
+    struct timespec ts = { .tv_sec = 0, .tv_nsec = (long)pollIntervalMs * 1000000L };
+    nanosleep(&ts, NULL);
+    waitedMs += pollIntervalMs;
+    // Optionally re-send dref registrations periodically
     registerDref(DREF_LATITUDE, "sim/flightmodel/position/latitude", 100);
     registerDref(DREF_LONGITUDE, "sim/flightmodel/position/longitude", 100);
     registerDref(DREF_ELEVATION, "sim/flightmodel/position/elevation", 100);
     registerDref(DREF_AGL, "sim/flightmodel/position/y_agl", 100);
+}
+if (!initialized) {
+    return false;
+}

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that the while (!initialized) loop is a busy-wait that can cause the application to hang indefinitely if the worker thread fails, which is a critical robustness issue.

High
Make init flag atomic

initialized is accessed from multiple threads without synchronization, causing a
data race. Make initialized atomic or guarded to ensure visibility and avoid
undefined behavior. Use volatile sig_atomic_t or C11 _Atomic bool depending on
compiler support.

src/main/target/SITL/sim/xplane.c [429-434]

-static bool initialized = false;
+#include <stdatomic.h>
+...
+static _Atomic bool initialized = false;
 static bool useImu = false;
 ...
 static void* listenWorker(void* arg)
 {
     ...
-    if (!initialized) {
+    bool expected = false;
+    if (atomic_compare_exchange_strong(&initialized, &expected, true)) {
         ENABLE_ARMING_FLAG(SIMULATOR_MODE_SITL);
         // Aircraft can wobble on the runway and prevents calibration of the accelerometer
         ENABLE_STATE(ACCELEROMETER_CALIBRATED);
-        initialized = true;
     }
     ...
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a data race on the initialized variable, which is accessed by two different threads without synchronization, leading to undefined behavior. This is a critical bug fix.

High
  • More

@MrD-RC
Copy link
Collaborator

MrD-RC commented Aug 17, 2025

Personally, I would say Initialized should be corrected to Initialised. Be careful when renaming variables. Make sure that no other areas were impacted.

@valeriyvan
Copy link
Author

Personally, I would say Initialized should be corrected to Initialised. Be careful when renaming variables. Make sure that no other areas were impacted.

I have kept original American spelling for Initialized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants