New Apps - Lot 1#27
Conversation
Trying to mix it up a little with some fun, useful and just weird. Adds more new apps... Brainfuck Breakout Magic 8 Ball Todo List Also re-organised the app name part of main.yml
|
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:
📝 WalkthroughWalkthroughThe PR adds four new applications—Brainfuck, Breakout, Magic8Ball, and TodoList—each with CMake project files, component source and header files, manifest.properties, and README documentation. It also updates the GitHub Actions workflow build matrix to include additional app_name entries: Brainfuck, Breakout, Magic8Ball, TodoList, and Todo. No other workflow steps were changed. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 6
🧹 Nitpick comments (5)
Apps/Brainfuck/manifest.properties (1)
7-10: Consider adding adescriptionfield for consistency.Both
BreakoutandTodoListmanifests include adescriptionkey.Brainfuckomits it, leaving the manifest incomplete relative to its siblings.✏️ Suggested addition
name=Brainfuck interpreter +description=Brainfuck esoteric language interpreterApps/TodoList/main/Source/TodoList.h (1)
26-27: Consider heap-allocatingitemsto reduce object footprint on RAM-constrained targets.
TodoItem items[MAX_TODOS]embeds ~6.6 KB (50 × ~132 bytes) directly in everyTodoListinstance. On ESP32 targets with limited heap, this is a notable fixed cost even when the app has few or zero tasks.♻️ Proposed refactor to dynamic allocation
- // Data - TodoItem items[MAX_TODOS] = {}; - int count = 0; + // Data + std::unique_ptr<TodoItem[]> items; + int count = 0;Initialize in
onShow/ constructor:+ items = std::make_unique<TodoItem[]>(MAX_TODOS);Apps/Breakout/main/Source/Breakout.h (1)
17-27:static constexprarray in a header causes per-TU duplication.The scalar constants on lines 17–24 are fine — they're optimized to immediates. However,
COLOR_SCORES[](line 27) is an array, andstatic constexprat namespace scope gives it internal linkage, meaning every translation unit that includes this header gets its own copy.If this header is included from multiple
.cppfiles, consider usinginline constexpr(C++17) to share a single definition across TUs.Suggested change
-static constexpr int COLOR_SCORES[] = { 50, 60, 70, 80, 90, 100, 110, 120 }; +inline constexpr int COLOR_SCORES[] = { 50, 60, 70, 80, 90, 100, 110, 120 };Apps/TodoList/main/Source/TodoList.cpp (1)
126-156: Hoist UI density computation out of the per-item loop.
lvgl_get_ui_density()andgetToolbarHeight()return the same values on every iteration. Computing them once before the loop avoids repeated calls.♻️ Suggested refactor
void TodoList::rebuildList() { if (!list) return; lv_obj_clean(list); if (count == 0) { lv_list_add_text(list, "No tasks yet. Add one below!"); } + UiDensity uiDensity = lvgl_get_ui_density(); + int toolbarHeight = getToolbarHeight(uiDensity); + int btnSize = (uiDensity == LVGL_UI_DENSITY_COMPACT) ? toolbarHeight - 8 : toolbarHeight - 6; + for (int i = 0; i < count; i++) { TodoItem* item = &items[i]; char display[MAX_TEXT_LEN + 8]; snprintf(display, sizeof(display), "%s %s", item->done ? LV_SYMBOL_OK : LV_SYMBOL_DUMMY, item->text); lv_obj_t* btn = lv_list_add_button(list, NULL, display); if (item->done) { lv_obj_set_style_text_opa(btn, LV_OPA_50, LV_PART_MAIN); } lv_obj_add_event_cb(btn, onItemClicked, LV_EVENT_SHORT_CLICKED, (void*)(intptr_t)i); - UiDensity uiDensity = lvgl_get_ui_density(); - int toolbarHeight = getToolbarHeight(uiDensity); - int btnSize = (uiDensity == LVGL_UI_DENSITY_COMPACT) ? toolbarHeight - 8 : toolbarHeight - 6; - lv_obj_t* delBtn = lv_button_create(btn);Apps/Brainfuck/main/Source/Brainfuck.h (1)
7-20: Consider the ~6 KBBfVMembedded in the class.
BfVMcontainstape[4096]+output[2048]≈ 6 KB. SinceBrainfuckobjects are heap-allocated viaregisterApp<Brainfuck>(), this is fine on ESP32. Just flagging for awareness in case the allocation strategy ever changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Apps/TodoList/main/Source/TodoList.cpp (2)
141-143: Hoist density/height/button-size out of the loop.
lvgl_get_ui_density(),getToolbarHeight(), andbtnSizeare recomputed on every iteration even though they are loop-invariant. Move them above theforloop (consistent with howonShowalready does this at lines 251-264).♻️ Proposed refactor
void TodoList::rebuildList() { if (!list) return; lv_obj_clean(list); + UiDensity uiDensity = lvgl_get_ui_density(); + int toolbarHeight = getToolbarHeight(uiDensity); + int btnSize = (uiDensity == LVGL_UI_DENSITY_COMPACT) ? toolbarHeight - 8 : toolbarHeight - 6; + if (count == 0) { lv_list_add_text(list, "No tasks yet. Add one below!"); } for (int i = 0; i < count; i++) { ... - UiDensity uiDensity = lvgl_get_ui_density(); - int toolbarHeight = getToolbarHeight(uiDensity); - int btnSize = (uiDensity == LVGL_UI_DENSITY_COMPACT) ? toolbarHeight - 8 : toolbarHeight - 6; ... }
176-177: Redundantlv_event_get_codeguard inonDeleteClicked.The callback is registered for
LV_EVENT_CLICKEDonly (line 156), so the runtime event-code check on lines 176-177 can never be false and is dead code.♻️ Proposed cleanup
void TodoList::onDeleteClicked(lv_event_t* e) { if (!g_instance) return; - lv_event_code_t code = lv_event_get_code(e); - if (code != LV_EVENT_CLICKED) return; - int idx = (int)(intptr_t)lv_event_get_user_data(e);
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
Apps/Brainfuck/main/Source/Brainfuck.cpp (2)
82-90:ciStrcmpreimplementsstrcasecmp.ESP-IDF / newlib provides
strcasecmpfrom<strings.h>. The hand-rolled ASCII loop can be replaced directly.♻️ Proposed refactor
+#include <strings.h> // for strcasecmp ... -static int ciStrcmp(const char* a, const char* b) { - while (*a && *b) { - int ca = (*a >= 'A' && *a <= 'Z') ? *a + 32 : *a; - int cb = (*b >= 'A' && *b <= 'Z') ? *b + 32 : *b; - if (ca != cb) return ca - cb; - a++; b++; - } - return (unsigned char)*a - (unsigned char)*b; -}Then at call sites replace
ciStrcmp(...)withstrcasecmp(...).
110-119: O(n²) bracket matching — consider a precomputed jump table.
bfFindBracketscans linearly on every[/]instruction. For a program with many nested loops this degrades to O(n²) before the cycle cap fires. A one-pass jump table (built once at the top ofbfRun) reduces each lookup to O(1).♻️ Sketch of a precomputed jump-table approach
-static int bfFindBracket(const char* code, int pos, int dir) { ... } void Brainfuck::bfRun(const char* code) { int len = strlen(code); + // Build bracket jump table O(n) + int* jumps = (int*)calloc(len, sizeof(int)); + if (jumps) { + int stack[512]; int top = 0; + for (int i = 0; i < len; i++) { + if (code[i] == '[') { stack[top++] = i; } + else if (code[i] == ']' && top > 0) { + int open = stack[--top]; + jumps[open] = i; jumps[i] = open; + } + } + } while (vm.pc < len && !vm.error) { ... case '[': if (vm.tape[vm.ptr] == 0) { - int target = bfFindBracket(code, vm.pc, 1); + int target = jumps ? jumps[vm.pc] : -1; ... } case ']': if (vm.tape[vm.ptr] != 0) { - int target = bfFindBracket(code, vm.pc, -1); + int target = jumps ? jumps[vm.pc] : -1; ... } } + free(jumps); }Apps/TodoList/main/Source/TodoList.h (1)
26-29: Add timer handle member to allow cancellation inonHide.
scheduleRebuild()creates anlv_timer_tbut discards the returned handle, leaving no way to cancel pending timers before the view is hidden. WhilerebuildList()has a guard (if (!list) return;) that prevents a crash, the timer callback still executes unnecessarily afteronHide()nulls thelistpointer.Store the timer handle as a class member and cancel it in
onHide()to avoid wasted callback invocations and follow the pattern established in other apps (Breakout, TamaTac).🔒 Proposed fix
In
TodoList.h, add:bool rebuildPending = false; + lv_timer_t* rebuildTimer = nullptr;In
TodoList.cpp—scheduleRebuild():void TodoList::scheduleRebuild() { if (rebuildPending) return; rebuildPending = true; - lv_timer_t* t = lv_timer_create(onDeferredRebuild, 0, this); - lv_timer_set_repeat_count(t, 1); + rebuildTimer = lv_timer_create(onDeferredRebuild, 0, this); + lv_timer_set_repeat_count(rebuildTimer, 1); }In
TodoList.cpp—onDeferredRebuild():void TodoList::onDeferredRebuild(lv_timer_t* timer) { TodoList* self = static_cast<TodoList*>(lv_timer_get_user_data(timer)); if (self) { + self->rebuildTimer = nullptr; self->rebuildPending = false; self->rebuildList(); } }In
TodoList.cpp—onHide():void TodoList::onHide(AppHandle app) { + if (rebuildTimer) { + lv_timer_del(rebuildTimer); // lv_timer_delete in LVGL v9.x + rebuildTimer = nullptr; + } list = nullptr; inputRow = nullptr; inputTa = nullptr; countLabel = nullptr; g_instance = nullptr; }
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Apps/Brainfuck/main/Source/Brainfuck.cpp (1)
349-351: Dead-code ternary operators — the fallback branches are unreachable after thelen < 3guard.Since line 347 already guarantees
len >= 3, the conditional expressions on lines 349–350 always take the true branch.♻️ Proposed simplification
- const char* ext3 = (len >= 3) ? &name[len - 3] : ""; - const char* ext2 = (len >= 2) ? &name[len - 2] : ""; + const char* ext3 = &name[len - 3]; + const char* ext2 = &name[len - 2];Apps/TodoList/main/Source/TodoList.cpp (1)
26-28:getToolbarHeightduplicatesBreakout.cpp— extract to a shared utility.The helper on lines 26-28 is functionally identical to
Apps/Breakout/main/Source/Breakout.cpp:99-105. If a third app repeats this pattern it becomes a maintenance liability.
|
Thank you! |
2 participants
Trying to mix it up a little with some fun, useful and just weird. Adds more new apps...
Brainfuck
Breakout
Magic 8 Ball
Todo List
Also re-organized the app name part of main.yml
Summary by CodeRabbit
New Features
Chores