Reduce voice RAM and ROM needs

[TheRealMoeder]

No description provided.

[TheRealMoeder]

Making progress, but still pretty heavy on the ROM:

devo7e without standard GUI:
ROM 104.92 kB RAM 9.75 kB

devo7e w/out standard GUI, HAS_EXTENDED_AUDIO
ROM 107.73 kB RAM 9.97 kB

devo7e w/out standard GUI, HAS_EXTENDED_AUDIO && HAS_MUSIC_CONFIG
ROM 108.69 kB RAM 9.97 kB

[TheRealMoeder]

more than 0.6 kB of ROM still goes to config system - looking forward to #626

[howard0su]

which function?

[TheRealMoeder]

Values below are the "bad" functions in devo7e with and without voice features. I guess MUSIC_PlayValue, TIMER_Update and TELEMETRY_Alarm are excused, since they add some new functionality. I was surprised at the cost for the two new muxes in MIXER_ApplyMixer.

  2080 CONFIG_WriteModel
   596 CONFIG_WriteTx
  2272 CONFIG_WriteModel
   612 CONFIG_WriteTx

   252 MIXER_ApplyMixer
   392 MIXER_ApplyMixer

   396 MUSIC_PlayValue

   340 TELEMETRY_Alarm
   596 TELEMETRY_Alarm

   472 TIMER_Update
   644 TIMER_Update

   280 ini_handler:hardware
  2492 ini_handler:model
   172 ini_handler:music
   712 ini_handler:tx
   336 ini_handler:hardware
  2724 ini_handler:model
   248 ini_handler:music
   740 ini_handler:tx

[TheRealMoeder]

This is probably as good as it gets for now. There are stil some minor issues with the voice implementation, but they'll get fixed in a different PR, as this one was about reducing ROM and RAM usage. I know the struct CustomVoice pretty pointless right now, but for the future I'm thinking of adding a flag for vibration, so this will become useful. Anyways, waiting for reviews and if satisfied, please squash.

[TheRealMoeder]

@PhracturedBlue the other cases won't work, because compiler complains about struct or variables not being defined. But if we do define them by excluding them from the pp if, thecompiler complains because they are not used

[TheRealMoeder]

Works like a charm on T8SGV2+

[TheRealMoeder]

@howard0su screenshot test fails:
target/tx/other/test/gui_helper.c:58: Screenshot Voice_config content mismatch!
Emulator looks fine to me though...

[TheRealMoeder]

OK.. Without any custom voice entries in voice.ini voice will currently be disabled, and thus voiceconfig page will show up different. I guess this behavior is not quite correct, as we could have global alarms defined in voice.ini, but no custom alarms. We then could still have voice working, but voice config menu is not available. Will have to think of a way to check that a valid voice.ini exists, but not quite sure how to do so yet...

[somewhatlurker]

Assuming this is all tested and works well, the code looks fine to me.... bear in mind that I'm pretty inexperienced and don't know the project well yet though.

I see that it only deals in external voice IDs now though... It's probably good, and half solves #999 in the process, but does introduce the small chance for issues loading old models with an abnormal voice.ini. (though it does make it essentially impossible to add support for gaps in numbering)

It would be possible to adopt a system of referencing voice.ini by line number rather than key names, which would behave more like the old system and allow for an almost identical solution to what I already proposed for #999. (models would take longer to load due to repeated config parsing, but it'd require benchmarking to figure out how significant it is)

[TheRealMoeder]

I think the code is significantly better with these changes, but far from good. Yes, I tested them thoroughly and didn't find any issues. Using line numbers is an interesting idea, so before introducing this potentially backwards compatibility breaking change, would you have time to use this branch as a base for reworking #999?

[somewhatlurker]

yeah, I'll try to have a look into it; or at least see how much work it would be to make behaviour identical to the old system for reimplementation of #1000 at another time.

[somewhatlurker]

Here's a commit that seems to make voice loading match the old behaviour: somewhatlurker@d34f6f4

Tested using a voice.ini containing the following, exhibiting misordered and missing ids:

[custom]
200=armed,548
201=disarmed,783
202=throttle hold on,1175
203=throttle hold off,1149
204=low rates,809
211=acro mode,835
;205=medium rates,1018
;206=high rates,783
207=angle mode 835
;saved hold0 alert as stability mode (207)
208=stability mode,1123
209=horizon mode,966
210=rate mode,679
212=flight mode 0,1253
213=flight mode 1,1149
214=flight mode 2,1097
215=flight mode 3,1175
...

Under both current master code and my branch (tested in zip_win_emu_t8sg_v2), HOLD0=207 in a model's voice section refers to "stability mode" (mp3 208).
Note that saving still uses the external IDs and thus the fix from #1000 would need to be implemented to even have a self-consistent system at this stage.

Another issue is that you can't reach the end of the list due to non-contiguous numbering. That could be fixed by either using the line number referencing in the voice config page or recording the maximum ID rather than number of IDs to voice_ini_entries. (the former would behave more like the old system)

[somewhatlurker]

An update on what I've been doing:

https://github.com/somewhatlurker/deviation/tree/voice_diet2 has been updated to use essentially the same system as #1000 to load both old and new entries correctly, however it uses the "+" prefix to at least maintain some level of support for loading new models in older firmware. The meaning of the prefix should probably be explained in the manual if these changes are used.

It also makes all IDs up to the maximum used one accessible in model voice config. IDs with no voice will have the label "[ID#] (missing)". (it might make sense to have this translatable, but I'm not sure how to do that)

[TheRealMoeder]

Your work looks fine. Translation should be triggered with _tr("string"). While your submission should work nice, I'm honestly not sure if it is worth keeping backward compatibility here. We never had an official release with voice, it was always a nightly feature (although that is what 99% uses). The Prefix stuff makes the code unnecessarily bloated, and as you suggested the new prefix is not self-explanatory. @goebish opinion?

I'd personally be fine with a breaking change if we could also add more config options for voice sounds, like setting additional flags for vibration and buzzer. Currently running both is crudely implemented through sound.ini for the global sounds, carried over from the original voice work not done by me.

[somewhatlurker]

Yeah, I see what you mean about bloat after checking the full diff for my changes. I suppose cherry picking 5776c82 and ef11399 (and just the voiceconfig_page.c changes from d34f6f4) to only fix actual issues would be fine if code size is a concern. (though retaining guaranteed compatibility would be nice)

I feel you on the features front, but the audio system still seems pretty daunting to me with all the stuff in there together. Obviously an entirely new design for everything would be best, but that's definitely not something I can help with when I don't even understand the full scope of current features well. Would you want to work on the design or is that just a hypothetical scenario right now?

[TheRealMoeder]

My time is limited, but the Music/Voice topic is actually not too big to tackle, so probably something we could easily optimize. For now, I think making a PR out of voice_diet2 with your patches sounds like good idea to me, after that we two (and maybe @eried if he has time) can discuss further development of voice/sound support in a separate issue.