Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR enhances the GPS conversion functions in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ros2bridge/src/gps.cpp (1)
310-326:⚠️ Potential issue | 🟠 MajorAdd validation guards for
msg.timebefore casting totime_t.The check
if (msg.time > 0)allows+infand oversized doubles through. Casting these totime_tis undefined behavior when the value doesn't fit. Both the GGA block (lines 310–326) and RMC block (lines 408–424) need to validate withstd::isfinite()and a range check againststd::numeric_limits<time_t>::max():Suggested fix
+#include <limits> ... - if (msg.time > 0) + if (std::isfinite(msg.time) && msg.time > 0 && + msg.time <= static_cast<double>(std::numeric_limits<time_t>::max())) { const auto time_t_val = static_cast<time_t>(msg.time);Apply to both the GGA (lines 310–326) and RMC (lines 408–424) blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ros2bridge/src/gps.cpp` around lines 310 - 326, Validate msg.time for finiteness and range before casting to time_t: replace the simple if (msg.time > 0) guard in both the GGA block (where time_t_val, utc_tm and gga.fields.UTCTime are set) and the RMC block with a check that std::isfinite(msg.time) && msg.time > 0 && msg.time <= static_cast<double>(std::numeric_limits<time_t>::max()); only perform the static_cast<time_t>(msg.time) and call gmtime_r/gmtime_s after that validation to avoid undefined behavior for +inf/oversized values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ros2bridge/src/gps.cpp`:
- Around line 217-219: The code only assigns msg.status.service =
obj.gnss_service_mask when fix_type != UNKNOWN, causing the fallback GGA mapping
to overwrite any valid gnss_service_mask; move or duplicate the logic that sets
msg.status.service from obj.gnss_service_mask (using the nonzero
check/static_cast<uint16_t> fallback to 1) so it runs regardless of fix_type
(e.g., assign msg.status.service from obj.gnss_service_mask before the fix_type
conditional or apply the same nonzero check in both branches); update the same
pattern for the other occurrence mentioned (the block around the second instance
analogous to lines 246-248) so gnss_service_mask is always propagated
independently of fix_type.
- Around line 238-240: The PPS fix (fix_quality == 3) is mapped inconsistently:
update the switch that sets msg.status.status (the case 3 branch in gps.cpp) so
it uses the same status value as ggaFixQualityToGpsStatus() does for case 3
(i.e., use STATUS_FIX instead of STATUS_SBAS_FIX) to make NavSatFix and GPSFix
exports agree for PPS observations; locate the case 3 branch that sets
msg.status.status and change it to the same enum/constant used by
ggaFixQualityToGpsStatus().
---
Outside diff comments:
In `@ros2bridge/src/gps.cpp`:
- Around line 310-326: Validate msg.time for finiteness and range before casting
to time_t: replace the simple if (msg.time > 0) guard in both the GGA block
(where time_t_val, utc_tm and gga.fields.UTCTime are set) and the RMC block with
a check that std::isfinite(msg.time) && msg.time > 0 && msg.time <=
static_cast<double>(std::numeric_limits<time_t>::max()); only perform the
static_cast<time_t>(msg.time) and call gmtime_r/gmtime_s after that validation
to avoid undefined behavior for +inf/oversized values.
jlblancoc
force-pushed
the
feat/new-gps-obs-fields
branch
2 times, most recently
from
fb57572 to
8c47bef
Compare
jlblancoc
force-pushed
the
feat/new-gps-obs-fields
branch
from
8c47bef to
d5360ba
Compare
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements