Conversation
Scale radar distance and speeds down by a factor of 4 to match vision.
There was a problem hiding this comment.
Thanks for contributing to opendbc! In order for us to review your PR as quickly as possible, check the following:
- Convert your PR to a draft unless it's ready to review
- Read the contributing docs
- Before marking as "ready for review", ensure:
- the goal is clearly stated in the description
- all the tests are passing
- include a route or your device' dongle ID if relevant
There was a problem hiding this comment.
🟡 Changes recommended
Bosch scaling must be isolated from Continental data, and the signal range must be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adjusts Tesla Bosch radar distance scaling by a factor of four to prevent false collision warnings.
Changes:
- Reduces radar signal scale factors.
- Applies the changes in the shared Tesla radar definition.
File summaries
| File | Findings |
|---|---|
opendbc/dbc/generator/tesla/_radar_common.py |
Critical (3 votes): Bosch-specific scaling also affects Continental radar data. Moderate (1 vote): The advertised range does not match the updated factor. |
Review details
Suppressed comments (4)
opendbc/dbc/generator/tesla/_radar_common.py:10
- These signals are unsigned (
@1+) and are decoded asraw * factor + offset. With the new LongSpeed factor, its 12-bit maximum decodes to4095 * 0.015625 - 128 = -64.015625, so every value is negative; the same offset/factor mismatch affects LatDist and LongAccel here. Real positive relative speeds, lateral positions, and accelerations therefore become incorrect negative values; adjust the offsets/factors consistently with the intended physical conversion.
SG_ LongSpeed : 12|12@1+ (0.015625,-128) [-128|128] "meters/sec" Autopilot
SG_ LatDist : 24|11@1+ (0.0078125,-128) [-128|128] "meters" Autopilot
SG_ ProbExist : 35|5@1+ (3.125,0) [0|96.875] "%" Autopilot
SG_ LongAccel : 40|10@1+ (0.0078125,-16) [-16|16] "meters/sec/sec" Autopilot
opendbc/dbc/generator/tesla/_radar_common.py:19
- LatSpeed uses the same unsigned raw-value formula with an offset of -64, so the new factor limits its 10-bit decoded range to [-64, -56.0078125].
radar_interface.pyassigns this signal directly toyvRel, making normal nonnegative lateral relative velocities decode incorrectly; the offset must be revised together with the factor.
SG_ LatSpeed : 0|10@1+ (0.0078125,-64) [-64|64] "meters/sec" Autopilot
opendbc/dbc/generator/tesla/_radar_common.py:20
- These changes are a 16x reduction (
0.125to0.0078125), not the stated 4x reduction. If these fields are meant to undergo the same correction, their factors should be quartered to0.03125(with their offsets/ranges adjusted consistently); otherwise they should not be changed as part of a longitudinal-distance fix.
SG_ LatSpeed : 0|10@1+ (0.0078125,-64) [-64|64] "meters/sec" Autopilot
SG_ Length : 10|6@1+ (0.0078125,0) [0|7.875] "m" Autopilot
opendbc/dbc/generator/tesla/_radar_common.py:6
- After changing the factor, the largest representable 12-bit value is
4095 * 0.015625 = 63.984375, but the DBC range still advertises255.9. Consumers or validators that use the signal range will receive an incorrect contract; update the upper bound to match the new factor.
SG_ LongDist : 0|12@1+ (0.015625,0) [0|255.9] "meters" Autopilot
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| SG_ LongDist : 0|12@1+ (0.0625,0) [0|255.9] "meters" Autopilot | ||
| SG_ LongSpeed : 12|12@1+ (0.0625,-128) [-128|128] "meters/sec" Autopilot | ||
| SG_ LatDist : 24|11@1+ (0.125,-128) [-128|128] "meters" Autopilot | ||
| SG_ LongDist : 0|12@1+ (0.015625,0) [0|255.9] "meters" Autopilot |
daggerhashjack
left a comment
There was a problem hiding this comment.
Requesting changes; this cannot be merged safely as written.
-
The unchanged offsets make the decoded kinematics invalid. In
_radar_common.py:7-10,19, reducing the factors without changing the offsets meansLongSpeedcan only represent -128 to -64.015625 m/s,LatDist-128 to -112.0078125 m, andLatSpeed-64 to -56.0078125 m/s. Zero relative velocity and a centered target become impossible. A production CANPacker/CANParser reproduction turns the existing zero-speed, zero-lateral-position values into -96 m/s and -120 m respectively. -
Three factors change by 16x, not 4x.
0.125 -> 0.0078125affects LatDist, LatSpeed and Length. A 4 m length decodes as 0.25 m. That is inconsistent with the proposed fourfold correction. -
This is not Bosch-only. Both
tesla_radar_bosch.pyandtesla_radar_continental.pyimport this helper. The patch also changes the Model 3/Y Continental radar definitions.
I also decoded retained raw Bosch CAN through the current and proposed definitions. Bus 1 frame 0x34c, bytes bd046a0bfc029e78, changes from distance 75.8125 m / relative speed -22 m/s / lateral position 1.375 m to 18.953125 m / -101.5 m/s / -119.9140625 m. All 9,940 valid, tracked A-message updates in that segment become lateral positions over 100 m from center under this patch.
Radar/vision disagreement does not establish a constant scaling error. Please provide raw frames with an independently known target range, scope any hardware-specific correction to that format, and include decoding regressions covering zero/positive/negative velocity and centered/lateral targets. The current failures need to be fixed before this is eligible to merge.
|
Closing without merging. The scaling changes leave the signed offsets inconsistent, producing invalid relative-speed and lateral-position values. We traced the reported braking issue to model-output decoding and radar association instead; those fixes are being handled separately. |
Adjusts the Tesla Bosch radar scaling down by a factor of 4. This fixes the false positive collision warnings that occur because the radar distance was reporting 4x further than the vision model.