-
-
Notifications
You must be signed in to change notification settings - Fork 354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SD card full warnings and functional degradation if SD card has less than 50MB free space #3730
Conversation
- checks SD card free space at startup, start of logging and writing screenshots - issues warning to user if SD card has less than 50MB free space - will not write logs or screenshots if free space is less than 50MB at activation timerReset - fixes crash of warning popup in logs.cpp (EdgeTX#3728)
I'm not sure whether it's a good idea to popup a full screen warning when the radio may be used to fly a model (screenshot or logging error). This could result in the user being distracted or panicking and a crashed model. At startup is fine; but perhaps the screenshot and logging checks should fail silently. |
I would also suggest adding detail to the TR_SDCARD_FULL string to tell users that screenshots (for color radios) and logging are disabled. |
The other places where POPUP_WARNING() is used in logs.cpp should probably also be changed as well. |
The card full check for logging is done only when the log file is opened. It is theoretically possible for the log writing to open the file and then fill the SD card while logging. Perhaps the check for free space should in logsWrite, and it should also close the log file if it then runs out of space while logging. |
Thanks for your feedback. The full screen warning is only used at startup. While operating it's the smaller popups overlaying a portion of the screen. I already changed all occurrences of the popup to UI_IN_TASK versions. Sure I can amend the popup texts. Just used what was already there trying to avoid new text to be translated. I thought about checking in logsWrite() too but wasn't sure about the time it takes to call for the sector count. I'll measure tomorrow and put the check in if the penalty is small enough to run it repeatedly. 50MB should be safe for another 3h of logging at the fastest rate but shutting down logging when the threshold is hit is clearly the better option. |
I think you do not check at every write. You know how many bytes each write writes and, normally, when logging, nothing else writes or can write huge amounts of data, because logging is not possible when USB is connected in storage mode and where should the data come from. So a quick sanity check every minute, or so, should be enough, when the limits leave enough room and then stop logging at 50MB free space left. |
Could be time-based, or interval based (i.e. check every 30 writes or something). As an initial "don't crash the radio" fix, a silent "failure" would be fine, but a proper implementation should inform the user that the function triggered did not run (and why)... I know I would be very annoyed if logging was not running and I didn't know, especially when it's that time you're doing a test flight and something goes wrong. Or a screenshot of GPS coordinates or something, and the screenshot is not taken. |
I have updated this PR to cover 3 scenarios of SD card full:
I measured the time it takes to query the SD card driver for the number of free sectors. It takes a negligible 5.2us so I poll the SD card driver every log write cycle, i.e. at the same rate logging is happening. This means a worst case of 5.2us every 100ms. The PR currently has 3 lines of code for testing scenario 2. It will start logging if the SD card is above threshold but mimic SD card full after 30 written log lines. To test in a real scenario comment lines 283-285, uncomment line 286 and make sure to start logging slightly above threshold. I have tested this PR as much as I can but very much appreciate more testing. @philmoz While testing with traces enabled I noticed some unexpected output upon closing the popup. The trace below shows the part right after calling POPUP_WARNING_ON_UI_TASK(), me trying to exit the popup by touch (works sometimes), me finally exiting the popup by using the RTN key and some unexpected complaints from the MPM and some garbled traces. I have tried multiple times. This is reproducible and happens right at exiting the popup. To me it looks like the popup writes to memory it shouldn't. This definitely needs some attention.
@gagarinlg PL18 - did you mean 8GB? |
No, I meant 8 MB, maybe it sas 16 MB. There is a similar chip on the PL18 as in the X10, TX16S, ... and no SD card slot. So flash memeory is very limited on that radio. |
No need to worry about the Pl18 in this PR, it's not a supported radio yet.
More an issue for the virtualfs or whatever the branch is that adds support
for it.
…On Sat, 1 July 2023, 5:18 am Malte Langermann, ***@***.***> wrote:
@gagarinlg <https://github.com/gagarinlg> PL18 - did you mean 8GB?
No, I meant 8 MB, maybe it sas 16 MB. There is a similar chip on the PL18
as in the X10, TX16S, ... and no SD card slot. So flash memeory is very
limited on that radio.
—
Reply to this email directly, view it on GitHub
<#3730 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJ66KNQRZOZG3E2N6KWLZDXN4Q7LANCNFSM6AAAAAAZXS3AJI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I can't test this at the moment; but I suspect it may be because the POPUP_WARNING_ON_UI_TASK function blocks the calling task until the popup is closed. You could try adding a parameter to the POPUP_WARNING_ON_UI_TASK function call to tell it not to do the wait loops waiting for the popup to be closed. Example code below, change the call in logging to pass false as the last parameter. popups.cpp
popups.h
|
@philmoz have you deleted your comment? Just read the email but wasn't showing here. Now it is. Thanks for your suggestion, will try. |
@philmoz I think calling the popup in non-blocking mode did the trick. Many thanks! |
@philmoz I think we learned blocking the software timer task is not a good idea. This is why I checked other software timer instances for using popups. Apart from logging it's luckily only telemetry doing it in a rare context and condition: radio/src/telemtry/telemetry.cpp: I think this should also use the newly created non-blocking version of POPUP_WARNING_ON_UI_TASK(). What do you think? |
That will cause the 'radio antenna defective' sound to be played every ten seconds even when the popup is showing. |
@pfeerick no problem this PR not making RC1 as it needs some more testing especially on b&w radios. And there is still code in for testing scenario 3 which I'd remove after confirmation. @ParkerEde would be nice if you gave this a go on your X7 and X10s The test scenarios to consider with SD card full
Here's some test sequences also valid for the 2.8 PR (#3739): Preparation: create SF's for logging and screenshots. Both SF's activated by a switch. Test case scenario 3:
Test case scenario 1 and 2 Preparation: to test this without messing with the SD card you can check SD card free space with the SD card connected to a computer. Change opentx.h, search for IS_SDCARD_FULL() and change 50 to a value well over the number of Megabytes free on your SD card. Build and flash firmware with the changed value.
|
@philmoz which will probably be the right response to a bad antenna warning. If 10s is to much of a nuisance the rate could be changed to maybe 20s or 30s. I just think blocking the software timer task is not a good idea as we learnt |
@mha1 Have now tested this PR. It now works great with SD card logs. The screen is switched back to bright on alarms and stick movements also make the screen bright again when the pop-up is open. |
@pfeerick Thanks for your review |
@pfeerick can you cherry-pick this version (2.9) into main? |
I won't know for certain until it's merged (as I'll then have a single commit to pull across) but none of the changes stand out as being problematic/likely to cause a conflict. |
ok, I'll save the effort then and keep my fingers crossed |
Michael, adopt the brace position! 😆 Can we get some translations for this please 😃 The first phrase is essentially the title, and then the second phrase is identical but needs different formatting between colorlcd and B&W UIs - i.e. three lines on the smaller 128x64 display whereas the 212x64 is wide enough. #define TR_SDCARD_FULL "SD card full"
#if defined(COLORLCD)
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\nLogs and Screenshots disabled"
#else
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\036Logs and " LCDW_128_480_LINEBREAK "Screenshots disabled"
#endif
|
DE
|
FR:
#define TR_SDCARD_FULL "SD carte pleine"
#if defined(COLORLCD)
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\nJournaux et
Impr. écran désactivé"
#else
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\036Journaux et
" LCDW_128_480_LINEBREAK "Impr. écran désactivé"
#endif
@ Peter: did you see my last message with a better translation proposal?
Le lun. 10 juil. 2023 à 11:01, Peter Feerick ***@***.***> a
écrit :
… Michael, adopt the brace position! 😆 Can we get some translations for
this please 😃
The first phrase is essentially the title, and then the second phrase is
identical but needs different formatting between colorlcd and B&W UIs -
i.e. three lines on the smaller 128x64 display whereas the 212x64 is wide
enough.
#define TR_SDCARD_FULL "SD card full"
#if defined(COLORLCD)
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\nLogs and Screenshots disabled"
#else
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\036Logs and " LCDW_128_480_LINEBREAK "Screenshots disabled"
#endif
[image: snapshot_01]
<https://user-images.githubusercontent.com/5500713/252286886-7c266d52-e29d-4096-adc7-d99ad2be2ab6.png>
[image: snapshot_01]
<https://user-images.githubusercontent.com/5500713/252288648-d2a06bd9-ebb5-4b4a-aedd-5a73ff252fa0.png>
[image: snapshot_01]
<https://user-images.githubusercontent.com/5500713/252288843-ef06e560-836a-4219-a3a0-e15ef84b7b75.png>
- @zyren <https://github.com/zyren> CN / TW
- @Eldenroot <https://github.com/Eldenroot> CZ
- @HThuren <https://github.com/HThuren> DA
- @ParkerEde <https://github.com/ParkerEde> / @TheIsotopes
<https://github.com/TheIsotopes> DE
- @Pat6874 <https://github.com/Pat6874> FR
- @offer-shmuely <https://github.com/offer-shmuely> HE
- @robustini <https://github.com/robustini> IT
- @ToshihiroMakuuchi <https://github.com/ToshihiroMakuuchi> JP
- @ulfhedlund <https://github.com/ulfhedlund> SE
- @ajjjjjjjj <https://github.com/ajjjjjjjj> PL
- @zandorsp <https://github.com/zandorsp> PT
—
Reply to this email directly, view it on GitHub
<#3730 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/A6HE7XNLRWA3Z2SLYO44CPLXPPAHZANCNFSM6AAAAAAZXS3AJI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
IT #define TR_SDCARD_FULL "SDCard piena" |
PT
|
DA |
CZ #define TR_SDCARD_FULL "Plná karta SD" |
@pfeerick how about Another option: |
PL
|
CN #define TR_SDCARD_FULL "SD卡已满"
#if defined(COLORLCD)
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\n日志和截屏功能将被禁用"
#else
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\036日志和截屏 " LCDW_128_480_LINEBREAK "将被禁用"
#endif TW #define TR_SDCARD_FULL "SD卡已滿"
#if defined(COLORLCD)
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\n日誌和截屏功能將被禁用"
#else
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\036日誌和截屏 " LCDW_128_480_LINEBREAK "將被禁用"
#endif |
SE:
```
#define TR_SDCARD_FULL "SD-kortet fullt"
#if defined(COLORLCD)
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\nLoggar och skärmklipp inaktiverade"
#else
#define TR_SDCARD_FULL_EXT TR_SDCARD_FULL "\036Loggar och " LCDW_128_480_LINEBREAK "skärmklipp inaktiverade"
```
|
Yeah, I was thinking something like that... or even just ordering so it was "screenshots and " "logs disabled", but had already pinged translators. Will probably still change it for English, individual translations will probably need to be tweaked over time to suite word lengths, etc. The spill-over still looks better/seemed more readable than on three lines 😆 |
#define TR_SDCARD_FULL "הדיסק מלא״ |
updated languages CN, CZ, DA, DE, FR, HE, IT, PL, PT, SE, TW JP still open |
@zyren @Eldenroot @HThuren @TheIsotopes @Pat6874 @offer-shmuely @robustini @ulfhedlund @ajjjjjjjj @zandorsp Thank you guys @ToshihiroMakuuchi Please respond if you find the time |
If you like ? DA |
IT |
…ce (#3730) * justification: #3666 - checks SD card free space at startup, start of logging and writing screenshots - issues warning to user if SD card has less than 50MB free space - will not write logs or screenshots if free space is less than 50MB at activation timerReset - fixes crash of warning popup in logs.cpp (#3728) * update: warning message if SD card fills up to threshold while logging * empty commit * call popup in non-blocking mode * make b&w compatible to non-blocking popup calls * 2nd attempt * Popup: - turns on backlight if dimmed due to inactivity setting - if backlight is dimmed due to inactivity setting backlight can turnd on again by any controls * #3599 * fixed b&w UI * updated languages CN, CZ, DA, DE, FR, HE, IT, PL, PT, SE, TW * chore: Update translations
…ce (#3739) * 2.8 port of #3730 (PR_SDCard_low_warning_2.9) * Popup: - turns on backlight if dimmed due to inactivity setting - if backlight is dimmed due to inactivity setting backlight can turnd on again by any controls * remove test supporting code * fix b&w UI * updated languages CN, CZ, DA, DE, FR, IT, PL, PT, SE, TW * chore: Update translations
JP #define TR_SDCARD_FULL "SD card full" |
…ce (#3730) * justification: #3666 - checks SD card free space at startup, start of logging and writing screenshots - issues warning to user if SD card has less than 50MB free space - will not write logs or screenshots if free space is less than 50MB at activation timerReset - fixes crash of warning popup in logs.cpp (#3728) * update: warning message if SD card fills up to threshold while logging * empty commit * call popup in non-blocking mode * make b&w compatible to non-blocking popup calls * 2nd attempt * Popup: - turns on backlight if dimmed due to inactivity setting - if backlight is dimmed due to inactivity setting backlight can turnd on again by any controls * #3599 * fixed b&w UI * updated languages CN, CZ, DA, DE, FR, HE, IT, PL, PT, SE, TW * chore: Update translations
…ce (#3802) * fix: Prevent functional degradation if SD has less than 50MB free space (#3730) * justification: #3666 - checks SD card free space at startup, start of logging and writing screenshots - issues warning to user if SD card has less than 50MB free space - will not write logs or screenshots if free space is less than 50MB at activation timerReset - fixes crash of warning popup in logs.cpp (#3728) * update: warning message if SD card fills up to threshold while logging * empty commit * call popup in non-blocking mode * make b&w compatible to non-blocking popup calls * 2nd attempt * Popup: - turns on backlight if dimmed due to inactivity setting - if backlight is dimmed due to inactivity setting backlight can turnd on again by any controls * #3599 * fixed b&w UI * updated languages CN, CZ, DA, DE, FR, HE, IT, PL, PT, SE, TW * chore: Update translations * chore: Remove magic number * chore: JP language update for SD card full (#3809) * fix: Don't spam simulator log with file close msgs --------- Co-authored-by: Michael <mha1@users.noreply.github.com>
justification: #3666
Changes:
Note for testing: It is best to use a very small SD card, e.g. 256MB and check free space. Create TEMP directory and save arbitrary data to push SD card free space below the 50MB threshold.