Show power source type on detailed screen in soldering mode (#1708) #1709
Conversation
| @@ -4,6 +4,39 @@ extern osThreadId MOVTaskHandle; | |||
| extern osThreadId PIDTaskHandle; | |||
| extern OperatingMode currentMode; | |||
|
|
|||
| int8_t getPowerSourceNumber(void) { | |||
There was a problem hiding this comment.
Probably worth moving this into its own file with matching header, since its no longer coupled to debug menu
There was a problem hiding this comment.
its no longer coupled to debug menu
Technically it still is but just reused in another place at the same time. Plus, I always thought that the less new files in a patch the better but it depends on a project I guess. So, to be clear though - are you sure that this demands creating a separate translation unit? Just asking, Captn. If so, then tell me the name of a new file and the location in the source tree - it's not mine ship after all ;)
There was a problem hiding this comment.
This could just be a powerSource.c file in the same folder; I view files more as wanting to be able to know what file to open when I first open the repo. So I dont like overly deep mess'es of files with similar names. But also I dont like burying a shared function in a file that sounds like it only does one thing.
To me the question would be "given the function getPowerSourceNumber; which of these files has it in" and thats not clear.
source/Core/Drivers/OLED.hpp
Outdated
| @@ -105,7 +105,7 @@ class OLED { | |||
| static void setBrightness(uint8_t contrast); | |||
| static void setInverseDisplay(bool inverted); | |||
| static int16_t getCursorX() { return cursor_x; } | |||
| static void print(const char *string, FontStyle fontStyle); // Draw a string to the current location, with selected font | |||
| static void print(const char *string, FontStyle fontStyle, uint8_t num = 0); // Draw a string (or only a number of chars from string if specified) to the current location, with selected font | |||
There was a problem hiding this comment.
num (or n in .cpp) is a fairly awkward parameter name and isnt self-documenting. What about maxLength as the argument?
There was a problem hiding this comment.
Ok, how about just length not to overstretch the declaration? Will it be fine?
There was a problem hiding this comment.
Well its more of a max limiter than an actual length; but if you keep the comment noting its a max, that should be fine.~
source/Core/Drivers/OLED.cpp
Outdated
| const uint8_t *next = reinterpret_cast<const uint8_t *>(str); | ||
| bool cut = n ? true : false; |
There was a problem hiding this comment.
Instead of this, could default the max length to 255, then just expand the check on line 415 to do while(next[0] && n--) sort of thing
There was a problem hiding this comment.
Very creative! Haven't thought about that.
|
Just one last initiative of mine - if you're unhappy, then tell me and I will add a new file. But since Although I see your logic in file naming and I understand it - it makes sense. So I'm sorry - I didn't mean to be rude nor stubborn in any way at all: just with open source projects without strict rules & regulations sometimes it's just not obvious how to make it better, that's it. |
|
No need to apologise; different opinions are good; I prefer discussion on a PR than having inflexible rules that drive people nuts. Will wait and see if any others comment for a day or so; If not I'm happy to merge it in then. |
UX perspective
But nasty hacks may apply, sorry :(
A few comments:
OLED::printmay look - it works for my own surprise and, what's more important - doesn't break any other visible OLED functionality.Waiting for a feedback crossing fingers.