-
Notifications
You must be signed in to change notification settings - Fork 41
Service & card configuration #30
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
base: main
Are you sure you want to change the base?
Conversation
|
Any thoughts @daniloc ? |
|
@TimBroddin Thanks for the nudge, this looks like the right direction, yes! Thanks for taking a swing here. so the way I generally do testing for something like this is to get it flashed and running, then leave it sitting on a specific card overnight. if I wake up and it's still on that card and still operating stably, I'll be feeling pretty good. Have you had a bit of time to observe these changes on the device? |
…ing and related methods
|
Let's see if it still works tomorrow 😀 |
|
Good news! No crashes! I'll fix a small UI issue that happened after the rebase this weekend, resolve conflicts, and will then move this from draft. |
|
Pfew ... summertime happened and I kept pushing forward rebasing this PR 🫠 I marked it as Ready for review now. I tested it and nothing seems to break. Could you try it as well @daniloc ? |
|
Thank you for your patience, @TimBroddin, been heads down on the last push to ship our kits. Having a look now but if I'm spotty it's because of drowning 🫠 |
src/ui/CardController.cpp
Outdated
| friendDef.factory = [this](const String& configValue) -> lv_obj_t* { | ||
| // Create new friend card (ignore configValue for now) | ||
| // Register FRIEND card type (no configuration changes) | ||
| CardDefinition friendDef(CardType::FRIEND, "Friend card", false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My biggest thing is: why does setting up cards look so different? The explicit properties being set (as previous) is much nicer to my eye than these (anonymous) arguments passed into the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, and this is probably due to my assistant Claude wandering off, and me not noticing. I'll fix this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good!
|
Pretty cool, but I'm not loving the changes to setting up cards. is that just the robot wandering off the path or do you have a case for why it's better? Only other quirk is how, if you have existing configuration, you'll boot into a weird state when you come back after updating. I need to spend a little more time digging into what that's about. I also found that adding a new insight worked, but it doesn't immediately load. Everything worked okay on reboot though and I don't know if that was a transient thing. Otherwise man this is a big improvement. I'll do a little noodling around with design and microcopy. |
…rity and consistency
…kHog into add/configuration
|
Hey @daniloc 👋 Congratulations on the ✨ real ✨ launch of DeskHog. The announcement email is rad! (Also, respect to @leonposthog for setting up all those devices!) I restored the initialization to its previous state. I spent some time on seamless migration, but somehow failed miserably (mainly due to bad time management). I'll find some time this week to make this work! |
tl;dr This PR replaces the previous configuration system with a new one. There are now three types of configuration:
Examples for registration
Features
This PR still needs (extensive) testing, but I wanted to put it out here to gather some feedback on the architectural changes @daniloc .