V1.13.20 - Updates

[ClutchplateDude]

• Code improvement to stability, multithreading, memory usage, user input, etc.

• Fixed a bug that caused Latitude to overwrite Longitude in the EEPROM code.
  Issues Found & Fixes Applied

  CRITICAL — Fixed

  ┌─────┬────────────────────────────────────────────────────────────────────────────────────────────────────┬────────────────────────────────┬────────────────────────────────────────────────────────────────────────┐
  │ # │ Issue │ Location │ Fix │
  ├─────┼────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼────────────────────────────────────────────────────────────────────────┤
  │ 1 │ EEPROM address collision — _LONGITUDE_ADDR_1 was 13 (same as _LATITUDE_ADDR_1), so writing │ EPROMStore.hpp:171 │ Changed to 15 │
  │ │ longitude silently corrupted latitude │ │ │
  ├─────┼────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼────────────────────────────────────────────────────────────────────────┤
  │ 2 │ UDP buffer overflow — 255-byte buffer with read(..., 255) then buf[len] = 0 writes past end when │ WifiControl.cpp:229-231 │ Buffer to 256, read limit to sizeof()-1 │
  │ │ full │ │ │
  ├─────┼────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼────────────────────────────────────────────────────────────────────────┤
  │ 3 │ Stack buffer overflow in DayTime::ToString() — unbounded strcpy/strcat chain into 32-byte achBuf │ DayTime.cpp:243-245 │ snprintf with remaining-size bound, using String(float,prec) for AVR │
  │ │ │ │ float compat │
  ├─────┼────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼────────────────────────────────────────────────────────────────────────┤
  │ 4 │ Stack buffer overflow in Declination::ToString() — same pattern into 32-byte achBufDeg │ Declination.cpp:87-94 │ Same approach: String(float,prec) + bounded snprintf with %s │
  ├─────┼────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼────────────────────────────────────────────────────────────────────────┤
  │ 5 │ sprintf overflow risk in Meade responses — fixed-size 10-20 byte buffers with %ld could overflow │ MeadeCommandProcessor.cpp (6 │ sprintf → snprintf(buf, sizeof(buf), ...), undersized buffers enlarged │
  │ │ on large position values │ locations) │ │
  ├─────┼────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼────────────────────────────────────────────────────────────────────────┤
  │ │ Non-atomic _mountStatus read-modify-write — volatile int modified with |= and &= (3 instructions │ │ All direct _mountStatus |=/&= routed through │
  │ 6 │ on AVR); ISR could read a torn value mid-write │ Mount.cpp (~30 locations) │ setStatusFlag()/clearStatusFlag() which now wrap with │
  │ │ │ │ noInterrupts()/interrupts() │
  ├─────┼────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼────────────────────────────────────────────────────────────────────────┤
  │ │ ISR-shared variables missing volatile — _guideRaEndTime, _guideDecEndTime, _trackerStoppedAt, │ │ │
  │ 7 │ _compensateForTrackerOff shared between ISR and main loop without volatile, allowing compiler to │ Mount.hpp:659-666 │ Added volatile qualifier │
  │ │ cache stale values │ │ │
  ├─────┼────────────────────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────┼────────────────────────────────────────────────────────────────────────┤
  │ 8 │ EndSwitch state shared between ISR and main loop without volatile — _state written in │ EndSwitches.hpp:30,33 │ Added volatile to _state and _posWhenTriggered │
  │ │ processEndSwitchState() (ISR), read in checkSwitchState() (main loop) │ │ │
  └─────┴────────────────────────────────────────────────────────────────────────────────────────────────────┴────────────────────────────────┴────────────────────────────────────────────────────────────────────────┘

  HIGH — Fixed

  ┌─────┬───────────────────────────────────────────────────────────────────────────────────────┬────────────────────────────────────┬─────────────────────────────────────────────────────────────────────────────────┐
  │ # │ Issue │ Location │ Fix │
  ├─────┼───────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────┤
  │ 9 │ Division by _trackingSpeed — if zero (corrupted EEPROM calibration), produces NaN/Inf │ Mount.cpp:2593, 3592, 4287 │ Guard _trackingSpeed > 0, fallback to 0.0F │
  │ │ that propagates to slew targets and long casts (undefined behavior) │ │ │
  ├─────┼───────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────┤
  │ 10 │ Gyro division by zero — atanf(x / sqrtf(AcY²+AcZ²)) with all-zero accelerometer │ Gyro.cpp:93-96 │ Replaced with atan2f(-AcX, sqrtf(...)) which handles zero denominator correctly │
  │ │ readings (disconnected sensor); also powf(x,2) wasteful and 2.0f/2.0f redundant │ │ (returns ±π/2); powf → xx; removed redundant multiply-divide │
  ├─────┼───────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────┤
  │ 11 │ Meade command out-of-bounds access — processCommand accessed inCmd[1] without length │ MeadeCommandProcessor.cpp (11 │ Added inCmd.length() guard at entry to processCommand (min 2) and all handler │
  │ │ check; 10 handler functions accessed inCmd[0] on potentially empty strings │ functions) │ functions (min 1); added length check on Az/Alt branch before inCmd[1] │
  ├─────┼───────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────┤
  │ 12 │ Guide pulse duration parsed garbage — manual (inCmd[2]-'0')*1000 + ... produced wrong │ MeadeCommandProcessor.cpp:1608 │ Replaced with inCmd.substring(2, 6).toInt() │
  │ │ values for non-digit characters │ │ │
  ├─────┼───────────────────────────────────────────────────────────────────────────────────────┼────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────┤
  │ 13 │ WiFi server/UDP memory leak — new WiFiServer/new WiFiUDP on reconnection without │ WifiControl.cpp:132-138 │ Added delete before re-allocation; initialized pointers to nullptr in header │
  │ │ freeing previous objects │ │ │
  └─────┴───────────────────────────────────────────────────────────────────────────────────────┴────────────────────────────────────┴─────────────────────────────────────────────────────────────────────────────────┘

[openastrotech-bot]

Looks like your PR has code that needs to be changed in order to meet our coding standards!
Here are your options:

1. Apply the patch that was generated by the job
   1. Click details under the failing clang-format check
   2. Click the Artifacts dropdown in the top right
   3. Download + unzip the clang-format-diff.patch file into the OpenAstroTracker-Firmware repo
   4. Run git apply clang-format-diff.patch to make the changes
   5. Commit and push up the formatted code
2. Run clang-format locally
   1. Run the command bash -c 'shopt -s nullglob globstar;GLOBIGNORE=./src/libs/TimerInterrupt/*; for i in ./{.,src/**,unit_tests,boards/**}/*.{c,cpp,h,hpp}; do clang-format -i $i; done'
   2. Commit and push up the formatted code

[julianneswinoga]

Can you ask the AI to split the changes into separate commits with description of the changes/fixes? I'd be a lot easier to review if we can understand the thought process and what problems it's solving 🙂

[ClutchplateDude]

Can you ask the AI to split the changes into separate commits with description of the changes/fixes? I'd be a lot easier to review if we can understand the thought process and what problems it's solving 🙂

I added a table of what it fixed in the description. I'd rather not split this into 13 separate PRs with 2 line fixes in each....

[julianneswinoga]

Can you ask the AI to split the changes into separate commits with description of the changes/fixes? I'd be a lot easier to review if we can understand the thought process and what problems it's solving 🙂

I added a table of what it fixed in the description. I'd rather not split this into 13 separate PRs with 2 line fixes in each....

Lol I'm not asking for separate PRs, just separate git commits. https://stackoverflow.com/a/35008008

[ClutchplateDude]

Unfortunately, I didn't tell it to do one at a time, I asked it to fix all critical issues ...

[julianneswinoga]

This does a heap allocation, it's probably better to do

if (inCmd.length() == 6 && isdigit(inCmd[2]) && isdigit(inCmd[3]) && isdigit(inCmd[4]) && isdigit(inCmd[5]))
...
    int duration = (inCmd[2]-'0')*1000 +
                   (inCmd[3]-'0')*100 +
                   (inCmd[4]-'0')*10 +
                   (inCmd[5]-'0');