TamaTac & SfxEngine + Sprite tools#24
Conversation
Fight me rabbit!
|
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:
📝 WalkthroughWalkthroughAdds a new TamaTac virtual pet application with build/config files (CMake, manifest), app entrypoint, and TamaTac app class. Introduces game model and logic (PetStats, PetLogic), UI views (Main, Menu, Stats, Settings, Cemetery, Achievements), two mini-games (PatternGame, ReactionGame), sprites/icons and sprite tooling, persistence, an SfxEngine audio library (header, implementation, SfxDefinitions, GPLv3), achievements/cemetery systems, and a README for the TamaTac project. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 13
🧹 Nitpick comments (31)
Tools/sprite_tools/spritedata2png.py (1)
59-73: Minor:hex_patternis recompiled on every iteration.The regex on line 69 is compiled inside the loop body, creating a new compiled pattern for every sprite array found. Move it outside the loop.
♻️ Suggested fix
arrays = OrderedDict() + hex_pattern = re.compile(r'0x([0-9A-Fa-f]+)') for match in pattern.finditer(header_text): name = match.group(1) data_str = match.group(2) - hex_pattern = re.compile(r'0x([0-9A-Fa-f]+)') values = [int(h.group(1), 16) for h in hex_pattern.finditer(data_str)] arrays[name] = valuesTools/sprite_tools/sprite2c.py (1)
275-280:Image.openwithout context manager — file handle is not closed.
generate_spritedata(line 231) correctly useswith Image.open(…) as img:, butprocess_filedoes not, leaving the file descriptor open until GC.♻️ Suggested fix
def process_file(filepath, name, frame_width, frame_height, transparent_color): """Process a single PNG file and return header content.""" - img = Image.open(filepath) - frames = convert_sprite(img, frame_width, frame_height, transparent_color) + with Image.open(filepath) as img: + frames = convert_sprite(img, frame_width, frame_height, transparent_color) print(f" {name}: {len(frames)} frame(s) from {os.path.basename(filepath)}") return generate_header(name, frames, frame_width, frame_height)Tools/sprite_tools/generate_placeholders.py (2)
405-415: Unused loop variablesdelayandloopin frame-data and AnimFrame loops.These variables are unpacked but unused in the first two loops (lines 406, 412), while they're needed only in the final
animatedSpritesloop (line 419). Use_placeholders for clarity.♻️ Suggested fix
# Frame data arrays - for name, frames, delay, loop in sprites: + for name, frames, _, _ in sprites: for i, frame in enumerate(frames): lines.append(format_array(name, i, frame)) lines.append("") # AnimFrame arrays - for name, frames, delay, loop in sprites: + for name, frames, _, _ in sprites: frame_refs = ", ".join(f"{{sprite_{name}_frame{i}}}" for i in range(len(frames))) lines.append(f"constexpr AnimFrame frames_{name}[] = {{ {frame_refs} }};")
12-17:rgb565here andrgb_to_rgb565insprite2c.pyare duplicated.Both
generate_placeholders.py(line 12) andsprite2c.py(line 86) implement the same RGB→RGB565 conversion with the same transparent-avoidance logic. Consider extracting to a shared module if these tools evolve together.Also applies to: 86-88
Libraries/SfxEngine/README.md (1)
103-112: Add a language identifier to the fenced code block.The file-structure code block is missing a language specifier. Using an empty language or a generic one like
textsatisfies the linter and improves rendering.📝 Proposed fix
-``` +```text Libraries/SfxEngine/Apps/TamaTac/main/Source/CemeteryView.h (1)
14-19: Consider adding default member initializers toPetRecord.If a
PetRecordarray is declared without callingloadRecords(or used partially), the uninitializedvalidfield could lead to reading garbage. Defensive defaults cost nothing here.📝 Proposed fix
struct PetRecord { - Personality personality; - LifeStage stageReached; - uint16_t ageHours; - bool valid; + Personality personality{}; + LifeStage stageReached{}; + uint16_t ageHours = 0; + bool valid = false; };Apps/TamaTac/README.md (1)
174-196: Add a language identifier to the fenced code block.The project structure code block is missing a language specifier.
📝 Proposed fix
-``` +```text TamaTac/Apps/TamaTac/main/Source/CemeteryView.cpp (2)
57-89: Record shifting leaves stale field data for invalid entries.When
srcValidis false, only thevalidflag is propagated to the destination slot—oldpers,stage, andagevalues linger in storage. This is safe today becauseloadRecordsgates reads onvalid, but it's a latent data-hygiene issue. Something to be aware of if the persistence format evolves.
13-34: Move enum-to-string mappings toPetStats.hto eliminate duplication.The conversion logic for
PersonalityandLifeStageenums to strings is duplicated across three files:CemeteryView.cpp(as static helper functions),StatsView.cpp(as local variable + array), andMainView.cpp(as local variable). Moving these toPetStats.has inline functions alongside the enum definitions will prevent divergence if enum values are added or modified.Libraries/SfxEngine/Include/SfxDefinitions.h (1)
556-585: Jingle sequences are the most complex — consider a brief inline comment on the musical intent.The LevelUp and GameOver jingles are the densest sequences (8-9 notes with layered voices). A one-line comment noting the musical pattern (e.g., "ascending major arpeggio" or "chromatic descent") would help future maintainers tune these without trial-and-error.
Apps/TamaTac/main/Source/StatsView.cpp (2)
110-119: DuplicatedstageNamemapping — consider extracting a shared utility.An identical
LifeStage-to-string switch exists inCemeteryView.cpp(seestageNamefunction). Extracting this into a shared helper (e.g., inPetTypes.hor a common utility header) would eliminate the duplication and reduce the risk of the two copies drifting apart.
138-143: Hardcoded personality names array with magic number5is fragile.The
Personalityenum inPetStats.hcurrently has exactly 5 members (Energetic, Lazy, Glutton, Cheerful, Hardy), and the array matches this order. However, if the enum is extended or reordered, this array and bounds check will silently break. Consider usingPersonality::Hardydirectly as the upper bound (a pattern already present inPetLogic.cpp:459), or add aPersonality::COUNTsentinel to the enum and derive the bounds check from it.♻️ Suggested approach
- if (personalityValue) { - const char* personalityNames[] = {"Energetic", "Lazy", "Glutton", "Cheerful", "Hardy"}; - int idx = static_cast<int>(stats.personality); - if (idx >= 0 && idx < 5) { - lv_label_set_text(personalityValue, personalityNames[idx]); - } - } + if (personalityValue) { + static constexpr const char* personalityNames[] = { + "Energetic", "Lazy", "Glutton", "Cheerful", "Hardy" + }; + int idx = static_cast<int>(stats.personality); + if (idx >= 0 && idx <= static_cast<int>(Personality::Hardy)) { + lv_label_set_text(personalityValue, personalityNames[idx]); + } + }Apps/TamaTac/main/Source/MenuView.cpp (1)
39-65: Consider extracting a helper for styled menu button creation.Each of the four menu items repeats the same 5-line styling block (
bg_colordefault/pressed,text_color). A small helper likeaddStyledMenuItem(menuList, icon, text, handler, this)would reduce ~20 lines of boilerplate and make adding/removing items trivial.Apps/TamaTac/main/Source/ReactionGame.cpp (2)
96-105: No timeout when target is shown — player can wait indefinitely.Once
showTarget()is called andphasebecomesTargetShown, there is no timer scheduled to expire the round. A player could leave the screen indefinitely. If this is intentional (lenient design), no action needed. Otherwise, scheduling a timeout (e.g., 3–5 seconds) aftershowTarget()that auto-fails the round would tighten the gameplay loop.
174-196: Dead enum value:Phase::FinalResultis listed but never entered.
showFinalResult()setsphase = Phase::Done(line 160), so thePhase::FinalResultcase at line 193 is unreachable dead code. Either remove the enum value or use it as the phase during the final result display (settingDoneonly on the subsequent timer tick).Libraries/SfxEngine/Source/SfxEngine.cpp (1)
25-38: Global mutable LFSR state is not thread-safe.
s_lfsrands_retro_lfsrare file-scope globals mutated on every noise sample. While currently only one audio task calls these, if the engine is ever instantiated twice or noise is triggered from another context, these will race. Consider making themVoice-level or instance-level members.Apps/TamaTac/main/Source/Achievements.h (1)
35-55: Missing deleted copy/move constructors — inconsistent with sibling views.
MenuViewandSettingsViewexplicitly delete copy constructor and copy assignment.AchievementsViewdoes not, creating an inconsistency. While not a bug (the class only holds raw pointers), copying anAchievementsViewwould produce a shallow copy with aliased LVGL pointers, which is almost certainly unintended.Suggested fix
class AchievementsView { private: TamaTac* app = nullptr; lv_obj_t* parent = nullptr; lv_obj_t* mainWrapper = nullptr; public: + AchievementsView() = default; + ~AchievementsView() = default; + + AchievementsView(const AchievementsView&) = delete; + AchievementsView& operator=(const AchievementsView&) = delete; + void onStart(lv_obj_t* parentWidget, TamaTac* appInstance);Apps/TamaTac/main/Source/SettingsView.cpp (1)
113-125: Redundant load-before-save — read the other setting from the UI instead.Both
onSoundToggledandonDecayChangedload all settings from Preferences just to get the other setting's value before saving. Since both widgets are members of the same view, you can read their state directly, avoiding the extra Preferences I/O and a subtle TOCTOU window:♻️ Simplified onSoundToggled
void SettingsView::onSoundToggled(lv_event_t* e) { SettingsView* view = static_cast<SettingsView*>(lv_event_get_user_data(e)); if (view == nullptr || view->soundSwitch == nullptr || view->app == nullptr) return; bool isChecked = lv_obj_has_state(view->soundSwitch, LV_STATE_CHECKED); - view->app->setSoundEnabled(isChecked); - bool soundEnabled; - DecaySpeed decaySpeed; - view->loadSettings(&soundEnabled, &decaySpeed); - view->saveSettings(isChecked, decaySpeed); + uint16_t selected = view->decayDropdown ? lv_dropdown_get_selected(view->decayDropdown) : 1; + view->saveSettings(isChecked, static_cast<DecaySpeed>(selected > 2 ? 1 : selected)); }Also applies to: 127-144
Apps/TamaTac/main/Source/PetStats.h (1)
162-162: Minor type inconsistency:PET_HAPPINESS_GAINisintwhile all other constants areuint8_t.All other action effect constants are
constexpr uint8_t, but this one isstatic constexpr int. SinceclampStattakesint16_t, it works, but this is inconsistent and thestatickeyword is redundant inside a namespace.♻️ Suggested fix
- static constexpr int PET_HAPPINESS_GAIN = 5; + constexpr uint8_t PET_HAPPINESS_GAIN = 5;Apps/TamaTac/main/Source/Achievements.cpp (1)
75-82:count == 10is fragile; consider>= 10.If the persisted
cleanCntis somehow corrupted or skips past 10 (e.g., due to a race or a prior bug), theCleanFreakachievement will never unlock. Sinceunlock()is idempotent (it checks the bit before writing), using>= 10has no downside and is more robust.♻️ Proposed fix
- if (count == 10) { + if (count >= 10) { unlock(AchievementId::CleanFreak); }Apps/TamaTac/main/Source/PetLogic.h (1)
31-34:checkHealth,checkEvolution, andapplyStatDecayare public but appear to be internal implementation details.These methods are called from
update()andloadState()within PetLogic itself. Exposing them publicly breaks encapsulation and could lead to inconsistent state if called externally out of sequence. Consider moving them to the private section.♻️ Proposed fix
// Action handlers void performAction(PetAction action); - // State checks - void checkHealth(); - void checkEvolution(); - void applyStatDecay(uint32_t elapsedSeconds); - // Getters ... private: + // State checks (internal) + void checkHealth(); + void checkEvolution(); + void applyStatDecay(uint32_t elapsedSeconds); + // Action implementations void feed();Apps/TamaTac/main/Source/TamaTac.cpp (2)
327-350: Inconsistent clamping:onReactionGameCompletedoesn't clampscorewhileonPatternGameCompletedoes.
onPatternGameComplete(Line 341) clamps the score withstd::max(0, std::min(score, PatternGame::MAX_ROUNDS)), butonReactionGameComplete(Line 329) passesscoredirectly. AlthoughapplyPlayResulthandles any value gracefully, the inconsistency suggests one was overlooked.♻️ Proposed fix
void TamaTac::onReactionGameComplete(int score, bool won) { if (petLogic) { - petLogic->applyPlayResult(score, ReactionGame::MAX_ROUNDS); + int clampedScore = std::max(0, std::min(score, ReactionGame::MAX_ROUNDS)); + petLogic->applyPlayResult(clampedScore, ReactionGame::MAX_ROUNDS); petLogic->saveState();
471-479:onResetClickedignoreslv_event_t*parameter — consider a null-guard for consistency.All other event handlers extract and null-check
user_datafrom the event. This handler doesn't use it sinceresetDialogIdis static, but adding a guard or a comment would maintain consistency and prevent surprises if the function signature changes.Apps/TamaTac/main/Source/Sprites.h (1)
84-86: No bounds checking ingetAnimSpriteandgetIcon.Both accessors index directly into arrays using unscoped enums, which allow implicit conversion from arbitrary integers. An out-of-range value would cause an out-of-bounds read. While callers appear to use valid enum values, a defensive check would be prudent — especially since
getInfo()in Achievements.cpp already does bounds checking for a similar pattern.🛡️ Proposed fix
inline const AnimatedSprite& getAnimSprite(SpriteId id) { + if (id < 0 || id >= PET_SPRITE_COUNT) return animatedSprites[0]; return animatedSprites[id]; } ... inline const Icon& getIcon(IconId id) { + if (id < 0 || id >= ICON_COUNT) return icons[0]; return icons[id]; }Also applies to: 138-140
Apps/TamaTac/main/Source/MainView.cpp (2)
493-521: Tight coupling:drawOverlaysdirectly accessesTamaTac::petLogicstatic member.Rather than taking a
PetLogic*orconst PetStats¶meter (as other methods do),drawOverlaysreaches into theTamaTacclass to readpetLogic. This creates an implicit dependency and makes the method harder to test or reuse. Consider passing the needed state as a parameter, consistent withgetSpriteForCurrentState.
557-572:onAnimTimeraccessesTamaTac::pendingResetUI— ensure this flag is safe across tasks.
pendingResetUIis set inTamaTac::onResult(which may run on a different task than LVGL) and read here on the LVGL task. Since it's abool(likely atomic on most architectures) and only transitionstrue→false, this is safe in practice, but making itstd::atomic<bool>would formalize the intent.Apps/TamaTac/main/Source/TamaTac.h (3)
1-27: Includes and global buffers look reasonable.The extern canvas buffers total ~16 KB of static RAM (72×72 + 12×16×16 in
lv_color_t). For an ESP32 app this is fine, but worth keeping in mind if memory pressure grows.Minor inconsistency: Line 10 uses
<freertos/freertos.h>(lowercase), whileSfxEngine.huses"freertos/FreeRTOS.h"(proper casing). On case-sensitive file systems (e.g., Linux CI), the lowercase form may fail to resolve if the SDK only provides the canonical capitalized header.Suggested fix
-#include <freertos/freertos.h> +#include <freertos/FreeRTOS.h>
60-70: Static members make TamaTac implicitly a singleton — consider documenting the constraint.All core state (
petLogic,sfxEngine,updateTimer,timerMutex, etc.) isstatic, so a secondTamaTacinstance would silently share and corrupt this state. TheonCreate/onDestroycode inTamaTac.cppdoes null-check-and-allocate, but there's no guard against concurrent instances.If the Tactility framework guarantees at most one
Appinstance per registered app, this is fine — but a brief comment here (e.g.,// Singleton — framework ensures single instance) would save future readers from puzzling over thestaticdesign choice.
122-129: Eightfrienddeclarations create tight coupling across all view/game classes.Every view and game class gets full private access. This works for a monolithic embedded app, but if any of these modules grow, consider introducing a narrow interface (e.g., an
AppActionsstruct or abstract base) that views call instead of reaching intoTamaTacinternals. No action needed now — just flagging for future maintainability.Libraries/SfxEngine/Include/SfxEngine.h (2)
277-292:volatileon shared settings fields — works in practice but not idiomatic C++.
masterVolume_,enabled_,polyphonicGateEnabled_, etc. are read by the audio task and written from the app thread. On ESP32 (Xtensa), single aligned 32-bit reads/writes are naturally atomic, sovolatile floatworks here. However,std::atomic<float>(withmemory_order_relaxed) would be more correct per the C++ memory model and would communicate intent to future maintainers.No urgency — the current approach is a common embedded pattern and is safe on this target.
136-142: Move operations are implicitly suppressed but not explicitly deleted.Copy is correctly deleted. Move constructor/assignment are implicitly suppressed because copy ops are user-declared. Since
SfxEngineowns RTOS handles (task_,msgQueue_), an accidental move would be dangerous. Explicitly deleting them makes the intent clear:Suggested addition
SfxEngine(const SfxEngine&) = delete; SfxEngine& operator=(const SfxEngine&) = delete; + SfxEngine(SfxEngine&&) = delete; + SfxEngine& operator=(SfxEngine&&) = delete;
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (20)
Apps/TamaTac/main/Source/CemeteryView.h (1)
14-19: Consider default-initializing allPetRecordmembers.
personalityandstageReachedare left uninitialized whenvalidisfalse. While the code consistently checksvalidbefore reading them, defensive initialization prevents surprises if a future caller forgets the guard.📝 Proposed fix
struct PetRecord { - Personality personality; - LifeStage stageReached; - uint16_t ageHours; + Personality personality = Personality::Energetic; + LifeStage stageReached = LifeStage::Egg; + uint16_t ageHours = 0; bool valid = false; };Apps/TamaTac/main/Source/PetLogic.cpp (2)
195-260: Dual-loop decay applies base rates before personality modifiers — intentional?The two separate loops (lines 207–216 for base decay, lines 220–255 for personality/day-night) mean base decay can zero out stats before personality bonuses offset them. If merged into a single per-tick loop, personalities like Energetic (+1 energy) would counteract decay each tick rather than being applied after stats are already floored. This makes the current approach slightly more punishing during long absences.
Not a bug — just worth confirming this is the intended gameplay behavior.
262-273:stats.health <= 0is always== 0foruint8_t.Since
healthisuint8_tandclampStatprevents underflow, the<= 0comparison can never be true for negative values. Using== 0would be semantically clearer, though functionally identical.Tools/sprite_tools/generate_placeholders.py (2)
406-416: Use_for unused loop variables.Ruff flags
delayandloopas unused in the loops at lines 407 and 413 (B007). Use_to indicate intentionally ignored values.📝 Proposed fix
# Frame data arrays - for name, frames, delay, loop in sprites: + for name, frames, _, _ in sprites: for i, frame in enumerate(frames): lines.append(format_array(name, i, frame)) lines.append("") # AnimFrame arrays - for name, frames, delay, loop in sprites: + for name, frames, _, _ in sprites: frame_refs = ", ".join(f"{{sprite_{name}_frame{i}}}" for i in range(len(frames))) lines.append(f"constexpr AnimFrame frames_{name}[] = {{ {frame_refs} }};")
427-434: Moveimport osto the top of the file.PEP 8 and common convention place all imports at module level. The late import on line 428 inside
main()is functional but unexpected.📝 Proposed fix
import math +import os T = 0xF81F # Transparent (magenta)Then remove
import osfrom line 428 insidemain().Libraries/SfxEngine/Include/SfxEngine.h (1)
285-296:volatilefor cross-thread shared state is functional but non-portable.On ESP32 (32-bit aligned accesses are atomic),
volatileworks for these simple load/store patterns. However,std::atomicwithmemory_order_relaxedwould provide the same zero-overhead guarantees while being formally correct per the C++ memory model. Consider this for future portability.Libraries/SfxEngine/Include/SfxDefinitions.h (1)
23-33: Inline private method defined in a separate header — ensure inclusion is restricted.
loadSoundis a private inline member function defined outside the class's main header. This works correctly as long asSfxDefinitions.his only included inSfxEngine.cpp. If accidentally included elsewhere, it would cause ODR violations or link errors if the definition diverges.A brief comment at the top (e.g.,
// Internal — include only from SfxEngine.cpp) would help prevent accidental misuse.Libraries/SfxEngine/Source/SfxEngine.cpp (1)
476-478: Best-effort silence flush after I2S error exit.If the audio task exits due to an I2S write error (line 471), the flush at line 478 writes to the same possibly-failing device. This is acceptable as a best-effort cleanup, but the error return is silently ignored. Consider guarding this flush:
Optional: skip flush if the loop exited on I2S error
+ // Flush silence (skip if we exited due to I2S error) + if (self->i2sDevice_ != nullptr) { memset(self->audioBuffer_, 0, sizeof(self->audioBuffer_)); i2s_controller_write(self->i2sDevice_, self->audioBuffer_, sizeof(self->audioBuffer_), &written, pdMS_TO_TICKS(50)); + }Tools/sprite_tools/spritedata2png.py (1)
127-128: UpdateImage.NEARESTtoImage.Resampling.NEARESTfor Pillow 9.1.0+ compatibility.
Image.NEARESTwas deprecated in Pillow 9.1.0 and scheduled for removal in 10.0.0. While it still works currently, replace it withImage.Resampling.NEARESTto eliminate deprecation warnings and ensure forward compatibility.Proposed fix
- img = img.resize((width * scale, height * scale), Image.NEAREST) + img = img.resize((width * scale, height * scale), Image.Resampling.NEAREST)Apply the same change at line 144.
Apps/TamaTac/main/Source/CemeteryView.cpp (2)
110-117: Potential buffer overflow insnprintffor record text.The
text[80]buffer holds"#%d %s %s %dd %dh". IfpersonalityToStringorlifeStageToStringreturn longer strings, this could truncate. With 5 entries and typical enum-to-string values this is likely safe, but consider increasing the buffer or verifying the max combined length of both string conversions.
34-66: Record shift leaves orphaned preference keys for invalid records.When
srcValidisfalse(line 46), only thevalidflag is copied to the destination slot — thepers,stage, andagekeys for that destination remain stale in persistent storage. This is functionally harmless sinceloadRecordsgates onvalid, but it means preferences accumulate stale data that's never cleaned up.Tools/sprite_tools/sprite2c.py (1)
119-134: Performance:getpixelin a tight loop is very slow for large sprites.
img.getpixel()is called per-pixel inside nested loops. For large spritesheets this will be noticeably slow. Usingimg.load()to get a pixel-access object, or converting to a NumPy array, would be significantly faster.Example using pixel access object
+ pixels = img.load() frames = [] for row in range(rows): for col in range(cols): frame = [] for y in range(frame_height): for x in range(frame_width): - px = img.getpixel((col * frame_width + x, row * frame_height + y)) + px = pixels[col * frame_width + x, row * frame_height + y] r, g, b, a = pxApps/TamaTac/main/Source/Achievements.h (1)
34-59: Achievement logic mixed into the view class.The static bitfield operations (
loadAchievements,unlock,hasAchievement, etc.) are pure data/persistence logic with no UI dependency, yet they live onAchievementsView. This couples callers (e.g.,TamaTac.cppcallingAchievementsView::unlock) to a UI class. Extracting them into a standaloneAchievementsutility class/namespace would improve separation of concerns.Not blocking, but worth considering if the achievement surface grows.
Apps/TamaTac/main/Source/Achievements.cpp (1)
13-26: VerifyachievementInfosarray size stays in sync withAchievementId::COUNT.The array has 12 entries matching the current
COUNT = 12, but there's no compile-time assertion to catch a mismatch if new achievements are added to the enum without updating the array.Add a static_assert
static const AchievementInfo achievementInfos[] = { {"First Feed", "Feed your pet"}, ... {"Night Owl", "Play at night"}, }; +static_assert(sizeof(achievementInfos) / sizeof(achievementInfos[0]) == static_cast<int>(AchievementId::COUNT), + "achievementInfos must match AchievementId::COUNT");Apps/TamaTac/main/Source/TamaTac.cpp (2)
17-27: All app state is static — prevents multiple instances and complicates testing.Every member (
petLogic,updateTimer,timerMutex,sfxEngine, etc.) is a static field. This is acceptable for a single-instance embedded app, but be aware it meansonCreate/onDestroyaren't truly re-entrant: a secondonCreatewithout a prioronDestroywill leak or reuse stale state (thenullptrguards help, but they also mean a second create silently reuses old state).
432-477: Timer callback performs persistence I/O and achievement unlocks — may stall the FreeRTOS timer service.
onTimerUpdateruns on the shared FreeRTOS timer-daemon task. It callspetLogic->saveState()and multipleAchievementsView::unlock()calls (each instantiatingPreferencesand doing I/O). If preferences I/O blocks (e.g., flash write), all other software timers in the system are stalled until this callback completes.Consider deferring the heavy work to a dedicated task or using
lv_async_callto bounce it to the LVGL thread.Apps/TamaTac/main/Source/Sprites.h (1)
92-138: ODR concern:constexpricon arrays have internal linkage, butinline icons[]references them.The
constexpr uint8_t icon_poop[](and siblings) have internal linkage in C++17, meaning each translation unit gets its own copy with a distinct address. Theinline const Icon icons[]array has external linkage and is deduplicated by the linker, but its initializer references the TU-local icon arrays — technically an ODR violation since the initializer produces different pointer values in different TUs.In practice the data is identical so this won't cause bugs, but for correctness, mark the icon arrays as
inline constexprto give them external linkage:Fix linkage
-constexpr uint8_t icon_poop[] = { +inline constexpr uint8_t icon_poop[] = { 0x00, 0x18, 0x3C, 0x7E, 0x7E, 0xFF, 0xFF, 0x00, };Apply the same
inlineaddition to all othericon_*arrays.Apps/TamaTac/main/Source/TamaTac.h (3)
24-26: Consider defining named constants for magic numbers in buffer dimensions.
72,16, and12appear as raw literals. Extracting them intoconstexprconstants (e.g.,kCanvasSize,kIconSize,kIconCount) would make the relationship between sprite assets and buffer sizes self-documenting and easier to keep in sync.
60-70: Static singleton state is fragile if lifecycle isn't strictly managed.All 10
staticmembers rely on the assumption that exactly oneTamaTacinstance exists. IfonDestroydoesn't fully zero out every static (or if a second instance is created before the first is destroyed), stale pointers (petLogic,sfxEngine,currentApp) will silently corrupt state. Consider either:
- Using a true singleton accessor with a guard, or
- Making these instance members (possibly behind an inner
SharedStatestruct allocated once and ref-counted).This isn't blocking for a single-app embedded target, but it's worth hardening if the framework ever permits concurrent app instances.
122-129: Eightfrienddeclarations couple every view/game to all private internals.Every friend class can access every private member — not just the subset it needs. If any view is refactored, the blast radius is the entire private surface. A narrower alternative is to pass a small context/interface (e.g., a
TamaTacContextstruct or abstract interface) to each view, exposing only what it requires.Low priority for a self-contained app, but worth considering if the view count keeps growing.
|
|
||
| wrapperWidget = nullptr; | ||
| toolbar = nullptr; | ||
| menuButton = nullptr; |
There was a problem hiding this comment.
Timer start/stop should be moved to onShow() and onHide().
onCreate() and onDestroy() should only contain things that you need while the app is not visible (e.g. another app like a dialog is in front of it) such as remember the state of the game.
(This is why the screensaver suffers when the app is running)
|
Thanks again! Great job! |
2 participants
Fight me rabbit!
Summary by CodeRabbit