fix(lua): playNumber with PREC2 - last decimal digit missing#7364
fix(lua): playNumber with PREC2 - last decimal digit missing#7364arkarkark wants to merge 5 commits into
Conversation
Tested with:
playNumber(123, UNIT_VOLTS, PREC2) -- says 1.23 Volts
playNumber(103, UNIT_VOLTS, PREC2) -- says 1.03 Volts
playNumber(100, UNIT_VOLTS, PREC2) -- says 1 Volt
playNumber(110, UNIT_VOLTS, PREC2) -- says 1.1 Volts
Just want to check this is the right approach and if you approve I'll go in and fix the other languages too,
before asking for final merge.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughplayNumber was changed across language TTS files to preserve a second decimal remainder for mode==2 (PREC2), expand the condition that emits the decimal-point path, and speak the second fractional digit when present. A few prompt-base constants and minor whitespace/EOF formatting were also adjusted. ChangesTTS Decimal Precision Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
radio/src/translations/tts/tts_en.cpp (1)
66-66: 💤 Low valueInconsistent indentation: tab vs spaces.
Line 66 appears to use a tab character for indentation while the surrounding code uses spaces. This should be consistent with the rest of the file.
Proposed fix
if (mode == 2 && rem2) { - PLAY_NUMBER(rem2, 0, 0); + PLAY_NUMBER(rem2, 0, 0); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/translations/tts/tts_en.cpp` at line 66, The line containing the PLAY_NUMBER(rem2, 0, 0); macro is indented with a tab while the rest of the file uses spaces; edit the line where PLAY_NUMBER(rem2, 0, 0) appears and replace the leading tab with the same number of spaces used elsewhere (match surrounding indentation), ensuring consistent whitespace for the PLAY_NUMBER invocation and similar nearby lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@radio/src/translations/tts/tts_en.cpp`:
- Line 66: The line containing the PLAY_NUMBER(rem2, 0, 0); macro is indented
with a tab while the rest of the file uses spaces; edit the line where
PLAY_NUMBER(rem2, 0, 0) appears and replace the leading tab with the same number
of spaces used elsewhere (match surrounding indentation), ensuring consistent
whitespace for the PLAY_NUMBER invocation and similar nearby lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2857ed04-6d95-4efd-9e58-3329ae4b1ee9
📒 Files selected for processing (1)
radio/src/translations/tts/tts_en.cpp
|
This would be nice. I had to avoid PREC2 when I wrote my telmetery lua and I just play 1 digit at a time. Special functions only plays 1 digit after the decimal also when you have it under 2 past the decimal. |
pfeerick
changed the title
pfeerick
changed the title
|
That would affect any value played, not only LUA ones, we certainly don't want by default more/longer audio message than they are today |
are there places where playNumber is called outside of LUA with a mode 2 (PREC2)? Would it be better to fix those places to call it with number/10 and PREC1 since that is what you actually want in that instance? that would make the internal api much more consistent and powerful for when you actually want two digits of precision. |
|
I've been looking around the code and the only place I see outside of the tts directory that playNumber is called is in that seemed suspiciously low but then I thought of Special functions which calls it "Play Value" which internally is called PLAY_NUMBER and that search yielded a lot more. even this fuction in Then I searched the code for PREC2 and all the instances were for displaying the value not speaking it. So, unless I missed something, I hope that ameliorates your concern about more numbers being read out? |
|
I'm implementing the other languages now. I think I've found a bug in the Hungarian also seems to have the same problem but I don't see a hungarian sound pack to confirm either way I've also found when I try and say too much in german it just stops. If I have a lua file tied to a button like this, I can easily test by reloading the lua function in the simulator (key f7) in German, Spanish, french, it says 1volt 1,1volt 1,03volt 1,23 Finnish seems to be trying to use English functions and doesn't have a sound pack not super sure about the Polish on the simulator stdout. working on other langauges continues... |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/translations/tts/tts_ru.cpp`:
- Around line 78-88: The div_t qr is computed from the original number then
number is modified when mode == 2 (PREC2), leaving qr stale; update the code so
that after you assign number = qr2.quot and rem2 = qr2.rem you recompute qr =
div((int)number, 10) before any use (e.g., before the if (qr.rem || (mode == 2
&& rem2)) branch and before PLAY_NUMBER calls and the unit-form logic that
inspects qr.rem), ensuring PLAY_NUMBER, qr.quot and any unit-form decisions use
the refreshed remainder/quotient values; keep the existing variables (qr, qr2,
rem2, mode, number, PLAY_NUMBER, RU_FEMALE_UNIT) but recalc qr after the PREC2
adjustment.
In `@radio/src/translations/tts/tts_ua.cpp`:
- Around line 77-87: qr is computed before number is reduced when mode == 2, so
decimal speech and unit-form selection use stale quot/rem; after the block that
handles mode == 2 (the div_t qr2 = div((int)number, 10); number = qr2.quot; rem2
= qr2.rem;), recompute qr = div((int)number, 10) (i.e., call div((int)number,
10) again into qr) before any use of qr.rem or qr.quot (including the
PLAY_NUMBER(...) call and the unit-form selection logic that references qr), so
that qr reflects the scaled number.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c311057-27d9-46cb-b189-d0f63ffe63ed
📒 Files selected for processing (19)
radio/src/translations/tts/tts_cn.cppradio/src/translations/tts/tts_cz.cppradio/src/translations/tts/tts_da.cppradio/src/translations/tts/tts_de.cppradio/src/translations/tts/tts_en.cppradio/src/translations/tts/tts_es.cppradio/src/translations/tts/tts_fr.cppradio/src/translations/tts/tts_he.cppradio/src/translations/tts/tts_hu.cppradio/src/translations/tts/tts_it.cppradio/src/translations/tts/tts_jp.cppradio/src/translations/tts/tts_ko.cppradio/src/translations/tts/tts_nl.cppradio/src/translations/tts/tts_pl.cppradio/src/translations/tts/tts_pt.cppradio/src/translations/tts/tts_ru.cppradio/src/translations/tts/tts_se.cppradio/src/translations/tts/tts_sk.cppradio/src/translations/tts/tts_ua.cpp
💤 Files with no reviewable changes (1)
- radio/src/translations/tts/tts_en.cpp
|
along with my comment here which I updated as I was working through it. I think this is ready for review. Most languages followed the same pattern so were easy to update. I tried them all in the simulator but only had language packs for: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
radio/src/translations/tts/tts_ua.cpp (1)
152-157: 💤 Low valueConsider simplifying the unit-form selection logic.
The condition
mode > 0 && qr.remis redundant. Sincetmpalready holds-1(the sentinel value) when the decimal path is taken (line 93-94, 100), you can simplify this to:if (unit) { UA_PUSH_UNIT_PROMPT(unit, tmp); }Both branches currently produce the same result because
tmp == -1whenever a decimal was spoken, making the explicit check unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/translations/tts/tts_ua.cpp` around lines 152 - 157, The unit-form selection has a redundant explicit check (mode > 0 && qr.rem); simplify by always calling UA_PUSH_UNIT_PROMPT(unit, tmp) when unit is non-null so the code uses the sentinel value already stored in tmp for decimal cases; update the block containing UA_PUSH_UNIT_PROMPT to remove the conditional and ensure tmp is relied upon (references: UA_PUSH_UNIT_PROMPT, tmp, qr.rem, mode).radio/src/translations/tts/tts_ru.cpp (1)
153-158: 💤 Low valueConsider simplifying the unit-form selection logic.
The condition
mode > 0 && qr.remis redundant. Sincetmpalready holds-1when the decimal path is taken (line 94, 101), you can simplify to:if (unit) { RU_PUSH_UNIT_PROMPT(unit, tmp); }The explicit check is unnecessary because
tmp == -1whenever a decimal was spoken.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/translations/tts/tts_ru.cpp` around lines 153 - 158, The unit-form selection can be simplified by removing the redundant explicit check and always passing tmp to RU_PUSH_UNIT_PROMPT; replace the conditional block that checks "if (mode > 0 && qr.rem)" with a single call to RU_PUSH_UNIT_PROMPT(unit, tmp) when unit is non-null, since tmp is already set to -1 for the decimal path (see variables unit, mode, qr.rem, tmp and the RU_PUSH_UNIT_PROMPT call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@radio/src/translations/tts/tts_ru.cpp`:
- Around line 153-158: The unit-form selection can be simplified by removing the
redundant explicit check and always passing tmp to RU_PUSH_UNIT_PROMPT; replace
the conditional block that checks "if (mode > 0 && qr.rem)" with a single call
to RU_PUSH_UNIT_PROMPT(unit, tmp) when unit is non-null, since tmp is already
set to -1 for the decimal path (see variables unit, mode, qr.rem, tmp and the
RU_PUSH_UNIT_PROMPT call).
In `@radio/src/translations/tts/tts_ua.cpp`:
- Around line 152-157: The unit-form selection has a redundant explicit check
(mode > 0 && qr.rem); simplify by always calling UA_PUSH_UNIT_PROMPT(unit, tmp)
when unit is non-null so the code uses the sentinel value already stored in tmp
for decimal cases; update the block containing UA_PUSH_UNIT_PROMPT to remove the
conditional and ensure tmp is relied upon (references: UA_PUSH_UNIT_PROMPT, tmp,
qr.rem, mode).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd35aaf4-855d-4598-b9f0-b997d5771da5
📒 Files selected for processing (2)
radio/src/translations/tts/tts_ru.cppradio/src/translations/tts/tts_ua.cpp
4 participants
Tested with:
playNumber(123, UNIT_VOLTS, PREC2) -- says 1.23 Volts
playNumber(103, UNIT_VOLTS, PREC2) -- says 1.03 Volts
playNumber(100, UNIT_VOLTS, PREC2) -- says 1 Volt
playNumber(110, UNIT_VOLTS, PREC2) -- says 1.1 Volts
Just want to check this is the right approach and if you approve I'll go in and fix the other languages too, before asking for final merge.
Fixes #5480
Summary of changes:
remember the remainder of the first division by 10 and then say that if it's appropriate.
Summary by CodeRabbit