Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature: Need a Brightness- button #284

Open
davepl opened this issue May 24, 2023 · 56 comments
Open

Feature: Need a Brightness- button #284

davepl opened this issue May 24, 2023 · 56 comments
Assignees
@davepl

Comments

@davepl
Copy link
Contributor

davepl commented May 24, 2023

At some polnt the "Brightness Down" button, second from the left in the top row of the remote, next to ON) used to dim the brightness. If you hit ON, it would set to max brightness and pin the effect on.

I think the top two buttons should be Brite- and Brite+ and I prefer that Brite+ just goes to max brightness, not steps up. But we need to move the "SET_EFFECT_INTERVAL to MAX" feature to another button, one of the spare buttons in the lower right.
@davepl davepl self-assigned this May 24, 2023
@smiliea
Copy link

smiliea commented Jun 2, 2023

I will look into this.

@robertlipe robertlipe mentioned this issue Jul 24, 2023
Open
@smiliea
Copy link

smiliea commented Aug 10, 2023

@robertlipe Very cool! I just got mine to connect to my home network. I had to do a full reset of my home router to get it to work after hours of reading the code. I really like the UI, however, I think Dave wants IR remote capability too. I will start working on it in the C++ code.

@robertlipe
Copy link
Contributor

Welcome! IR support is definitely there. I use it frequently. I have some work pending in that code that I'll clean up and submit some day.

It's odd that even the 44 key remote don't really have "obvious" bindings for some of the things I think are obvious. I'm about to put a sticker on mine and rename some of the dedicated color keys to things I need more.

@smiliea
Copy link

smiliea commented Aug 10, 2023

I agree on the key bindings and labels. I'd like to see what you have already done so I don't duplicate your work!

@robertlipe
Copy link
Contributor

robertlipe commented Aug 10, 2023 via email

@smiliea
Copy link

smiliea commented Aug 18, 2023

Thanks for the great ideas to get me going, and I am glad to help out as much as I can. I love working with embedded systems because I like how you are both designing hardware and software at the same time. I just ordered a 44 key remote to test the code with a 44 key remote. I somehow have been working on an old copy of the git repo. However, this helped me spend some time getting up to speed on this codebase.

Personally, I like flat text files to store settings. they are easy to read in C++ and can be manually modified.

I think we should read key mappings from a IRRemoteSettings.ini text file during boot. This way someone could personalize their remote without having to change and compile the code, and it keeps things as simple as possible.

To expand further, I think it is possible (only if you have a LED matrix panel) to have an effects page that displays the IR code for the last key pressed on the remote. This could be used to help someone map out their remote. I am also sure we could do this mapping process in an automated way using the matrix display and writing the mapping file on the fly. From a procedural point of view, the display would show your current key for 2-5 seconds and then ask the user what key you would like the current key to go, and update the text file to store it.

I think tying the webserver and the IR together could be done, however, what if a user doesn't want a webserver? What if they are installing this in a location which can't broadcast WiFi (hospital)? Personally, I would like to keep these separate for these reasons. I come from a background writing code while working in rooms without outside internet access, so I may bring a different perspective. I have to rely on my memory, books, or the built-in help tools provided with whatever IDE I am using :) The late eighty's never left the building!

I also would like to develop a brightness box which shows up on the mesmerizer to indicate the current brightness level. It would be overlaid on top of the current effect, and would time out after a few seconds of inactivity.

I will spend some time this weekend taking a look at this code.

@robertlipe
Copy link
Contributor

robertlipe commented Aug 19, 2023 via email

@robertlipe
Copy link
Contributor

robertlipe commented Aug 19, 2023 via email

@smiliea
Copy link

smiliea commented Aug 23, 2023

@robertlipe I too have been thinking too much about remotes! The generic 44 key remote I ordered showed up yesterday, so I will look at it this week when I get a chance.

That is a great idea to use the web interface to select and set up the IR remote. I am going to need to study more on how the webserver code and API works. From what I remember browsing through the code and comments, I believe the API uses REST calls. This is way out of my comfort zone writing C and C++ data collection and analysis code and will be a good learning experience!

@robertlipe
Copy link
Contributor

robertlipe commented Aug 23, 2023 via email

@smiliea
Copy link

smiliea commented Aug 27, 2023

Thanks! I have been playing with the REST API. There are some very interesting things that can be done with this.

I didn't understand school until I realized it is the gymnasium for your brain. I was a junior in high school when I realized this (27 years ago), and it took me to college with a masters degree in electrical and computer engineering. I still take graduate school classes to this day to keep me up to date and keep the brain sharp. The last class I took was on Inertial Navigation and I was introduced formally to rotation matrices, Euler angles and quaternions. It is amazing what the MEMS IMU in your phone can do, especially when combined with a GNSS source using a Kalman filter!

@davepl
Copy link
Contributor Author

davepl commented Aug 27, 2023 via email

@robertlipe
Copy link
Contributor

robertlipe commented Aug 27, 2023 via email

@davepl
Copy link
Contributor Author

davepl commented Aug 27, 2023 via email

@robertlipe
Copy link
Contributor

robertlipe commented Aug 28, 2023 via email

@smiliea
Copy link

smiliea commented Aug 28, 2023

It’s inside the orange housing, and listed on the label on the bottom. How or why they do it one way or the other is a hardware problem :-) The S3 can also act like a flash drive when plugged in, right? That’s a cool feature! I didn’t know there were high pin count versions. I heard somewhere that it doesn’t do DMA, though, which is needed for the HUB75 matrix. Not 100% certain though. - Dave
…

@davepl
Dave, I have played around with the IMU a little bit after getting my hands on the M5Stick. The only way I knew it had one is because of the demo code. I didn’t know about the flash drive - that’s really cool.

I was thinking about building a LED light ball with a M5 in the middle that uses the IMU to keep the lights in a fixed position no matter how it spins. It could even display text as you toss it around! The NightDriver library would work well for this. How about an audio spectrum analyzer you can toss around with your friends?

@robertlipe

Andrew, if you yearn for GNSS + Kalman hacking, I could use a hand with my other hobby project: https://github.com/GPSBabel/gpsbabel/blob/robertlipe-kalman/kalmanfilter.cc - I'm secure enough in my programming chops to admit that the math in it makes my head hurt. I "just" needed a way to get rid of the tuned hardcoded constants and make it generally flexible enough to automatically throw out those four consecutive points that zing you from here to Cuba to Null Island and back in four second span.

Robert,

I will definitely check this project out, and see if I can assist you with the Kalman filter. It may require me to write down all of the equations being solved and double check the math. This absolutely can get complicated quickly! If you accidentally rotate out of order, you can get stuck forever. I will have some time this week after work and this weekend to check it out.

Thanks!

@robertlipe
Copy link
Contributor

robertlipe commented Aug 29, 2023 via email

@davepl
Copy link
Contributor Author

davepl commented Aug 29, 2023 via email

@revision29
Copy link
Contributor

I didn’t read all of the comments in this thread but skimmed enough to see the general direction of conversation. Last week I got my first esp32 board (Heltec WiFi kit v3) and some LEDs. Once I got NightDriver running and configured, I wanted to play around with remote functionality. An IR receiver diode was sourced from a dead DVD player. I changed the code for the 44 key remote to “true.” We all know how that didn’t work. Long story: I got all of the key codes for my remote mapped, declared some stuff to make the compiler happy, and defined colors for each button.

This lead me to discover that when a strip should be filled with a single color, it was failing. Turns out an effect was being added instead of the color being drawn. I suggest for general use this be changed. It iterates the color array and passes the value, but calls a different function.

I do have a suggestion that I would gladly try to help with. It’s my opinion that instead of iterating the color array to see if the button pressed matches an entry in the array, each button gets it own dedicated else if section. As inelegant and ugly as that will be, it would allow tinkerers to define whatever action they want. They can comment out the line that would fill the color and make it do whatever they want. It would, however, have a lot of seemingly redundant calls to the color fill method for 12-16 buttons. It makes me sick thinking about it, but would give the greatest flexibility for customization.

I also want to get the functionality of each button working as intended. So brightness would go up or down. The rgb colors could be adjusted on the fly. Fade 3 would loop through three pre-defined effects in a loop with a gentle fade between them. You get the picture.

Would such a contribution to the code be desired? I have 0 experience with C++ besides tinkering with NightDriver in the last week, but think I can manage well enough. It might take me a month or two with my schedule. I suppose I could just fork and do a pull request later and see if it gets accepted.

A major caveat is that whatever remote a person chooses, they will probably need to capture the key codes themselves. That’s easy enough with VS Code and console monitoring. If someone is tinkering at this level they should be able to handle that.

One last thing that’s related: how does one programmatically change the brightness of the LEDs? I’ve tried to make sense of the code to see what I could do to “dim” the LEDs but have come up short. I’ve only seen where the max brightness is calculated based on the max power draw. This is more for eye comfort while coding and getting things to work.

@robertlipe
Copy link
Contributor

robertlipe commented Sep 11, 2023 via email

@smiliea
Copy link

smiliea commented Sep 11, 2023

@revision29 , I will absolutely be here to bounce ideas and code off of me. And @robertlipe, it would be great to have you as a mentor!

There are a few things about the remote functionality I don’t care for fight now, like the brightness up/down changing effects. I think these buttons should only do brightness things. I also want to have a user definable IR button map that can be modified using the Web interface or a flat text file. We can discuss this further in Dave’s Garage discord server.

@revision29
Copy link
Contributor

Thanks for the feedback and following up. Life has gotten in the way and I have not responded. I had spent a good deal of time writing up a response and the wrong swipe on my trackback triggered my browser to go back in history and it trashed my comment without being so kind as to give me a warning. I will follow up later. The short version is that I have significantly reworked the remote code to have a user remote object with a vector of user define button objects. My underlying thought process is that one day this code will need to be compatible with having a web interface setting / changing these.

Anyway, later today I will retype my comment with code examples in an external markdown editor and then paste in the comment box so that Safari does not eat my comment again. That's what I get for trying to format my code for markdown in a web browser.

@revision29
Copy link
Contributor

Here is a followup to demonstrate what I have done. Keep in mind, this is my first dabbling with C++. I have worked with PHP, JavaScript, SwiftUI, MySQL, and a small amount of perl. I can google and consult stackoverflow like most every other programmer. There are some optimizations to be made once I get more familiar with the language and what can be simplified. Right now my focus is on basic functionality and establishing the main logic.

I have not found to increase brightness, yet. There is a CRGB method to dim a color, but that does math on each of the R, G, and B values of a specific color instance. It gets messy depending on the color. I fell down a couple of rabbit holes late at light trying to figure this out and need to revisit when I have time and a clear mind.

What I have hacked up so far:

New file: userremote.h

enum ButtonActions{
	/* 
	There are / will be definitions for all of the usual actions on 24 and 42 key remotes.
	It is understood that this will need to be reworke slightly to be compatible with action assignments being passed through a web interface.
	*/
    BRIGHTNESSUP,
    BRIGHTNESSDOWN,
    POWERON,
    POWEROFF,
    FILLCOLOR,
    TRIGGEREFFECT,
    CHANGER,
    CHANGEG,
    CHANGEB,
	...
};



/* 
This is the class for a user definable remote button.
Of note is the actionArgs property. 
It is assumed that at some point a user on a web interface will pass 
multiple comma separated arguments through this variable. As of now, I
am just using it to pass a string representing a hex color code.
*/
class RemoteButton
{
    public:
        String name;
        int keyCode;
        ButtonActions buttonAction;
        String actionArgs;
        RemoteButton (String name, int keyCode, ButtonActions buttonAction, String actionArgs) {
            this->name = name;
            this->keyCode = keyCode;
            this->buttonAction = buttonAction;
            this->actionArgs = actionArgs;
        };
};

This file also contains some declarations for code used to create a CRGB color from a hex string (not hex integer).

The UserRemoteControl class holds all of the button definitions. It also has a property declaring how many buttons this has. That will mostly be useful for when a person is creating a user remote in a web interface.

class UserRemoteControl {
    public:
        //Eventually, the button type and / or button count will depend on a user setting / config file.
        std::vector<RemoteButton> buttons; 
        void getRemoteButtons();
        int buttonCount;
        UserRemoteControl (int buttonCount = 0) {
            std::vector<RemoteButton> myButons {};
            this->buttons = myButons;
            getRemoteButtons();
        };

};

New file userremote.cpp

Again, will ignore the implementations for the string->hex->CRGB object conversion.

This is the implementation of the UserRemoteControl class getRemoteButtons method. Eventually, this can be retooled to get the arguments from a config file.

void UserRemoteControl::getRemoteButtons() {
	//You can also defien each action, color, etc. based on your layout.
	//Row 1
	this->buttons.push_back(RemoteButton ("Brightness Up",0xFF3AC5,ButtonActions::BRIGHTNESSUP, ""));
	
	this->buttons.push_back(RemoteButton ("Brightness Down",0xFFBA45,ButtonActions::BRIGHTNESSDOWN, ""));
	
	this->buttons.push_back(RemoteButton ("Power On",0xFF827D,ButtonActions::POWERON, ""));
	
	this->buttons.push_back(RemoteButton ("Power Off",0xFF02FD,ButtonActions::POWEROFF, ""));
	
	//Row 2
	this->buttons.push_back(RemoteButton ("Full Red",0xFF1AE5,ButtonActions::FILLCOLOR, "FF0000"));
	
	this->buttons.push_back(RemoteButton ("Full Green",0xFF9A65,ButtonActions::FILLCOLOR, "00FF00"));
	
	this->buttons.push_back(RemoteButton ("Full Blue",0xFFA25D,ButtonActions::FILLCOLOR, "0000FF"));
	
	this->buttons.push_back(RemoteButton ("Full White",0xFF22DD,ButtonActions::FILLCOLOR, "999999")); //because we don't want FULL white
	
	//Row 3
	this->buttons.push_back(RemoteButton ("Color 1",0xFF2AD5,ButtonActions::FILLCOLOR, "E18E28"));
	
	this->buttons.push_back(RemoteButton ("Color 2",0xFFAA55,ButtonActions::FILLCOLOR, "1B9205"));
	
	this->buttons.push_back(RemoteButton ("Color 3",0xFF926D,ButtonActions::FILLCOLOR, "170C96"));
	
	this->buttons.push_back(RemoteButton ("Color 4",0xFF926D,ButtonActions::FILLCOLOR, "F1BFDC"));
	
	...
	
	
	//Row 8
	
	this->buttons.push_back(RemoteButton ("Increase Red",0xFF08F7,ButtonActions::CHANGER, "10"));
	
	this->buttons.push_back(RemoteButton ("Increase Green",0xFF8877,ButtonActions::CHANGEG, "10"));
	
	this->buttons.push_back(RemoteButton ("Increase Blue",0xFF48B7,ButtonActions::CHANGEB, "10"));
	
	this->buttons.push_back(RemoteButton ("Quick",0xFFC837,ButtonActions::QUICK, ""));//fade speed is fast

	
	...
}

Changes to remotecontrol.cpp

Added a line above the void RemoteControl::handle() method implementation to create an instance of UserRemoteControl.

UserRemoteControl myRemoteController (44); // My remote has 44 buttons

Removed code handling the button action after the line:

auto &effectManager = g_ptrSystem->EffectManager();

Added in my own button handling code:

auto keyCodeMatch = std::find_if(
        myRemoteController.buttons.begin(), 
        myRemoteController.buttons.end(),
        [&result](const RemoteButton& potentialButton) { 
            return potentialButton.keyCode == result;
            }
        );

if (keyCodeMatch != myRemoteController.buttons.end())
        {
        //debugV("We have a matching keycode 0x%08X \n", result);
        auto index = std::distance(myRemoteController.buttons.begin(), keyCodeMatch);
        RemoteButton thisButton = myRemoteController.buttons[index]; 
        
        //debugI("We have a matching keycode 0x%08X the button is %s \n", result, thisButton.name);
        switch (thisButton.buttonAction){
            
            case ButtonActions::BRIGHTNESSUP:
                //To be determined
            break;
            case ButtonActions::BRIGHTNESSDOWN:
                lastManualColor.fadeLightBy(25); // sort of work but is messy and does not work well.
                effectManager.SetInterval(0);
                effectManager.SetGlobalColor(lastManualColor);
            break;
            case ButtonActions::POWERON:
                effectManager.ClearRemoteColor();
                effectManager.SetInterval(0);
                effectManager.SetGlobalColor(lastManualColor);
                effectManager.StartEffect();
                g_Values.Brightness = 200; //we don't want to cause damage or hurt eyes. Margin for going up and down.
            break;
            case ButtonActions::POWEROFF:
                effectManager.SetGlobalColor(CRGB(0,0,0));
                g_Values.Brightness = std::max(0, (int) g_Values.Brightness - BRIGHTNESS_STEP);
                effectManager.ClearRemoteColor();
            break;
            case ButtonActions::FILLCOLOR:
                lastManualColor = hexToCrgb(thisButton.actionArgs);
                effectManager.SetGlobalColor(lastManualColor); 
                effectManager.SetInterval(0);
            break;
            case ButtonActions::CHANGER:
                if (lastManualColor.red + thisButton.actionArgs.toInt() > 255 ) {
                    lastManualColor.red = 255;
                } else if (lastManualColor.red + thisButton.actionArgs.toInt() < 0) {
                    lastManualColor.red = 0;
                } else {
                    lastManualColor.red += thisButton.actionArgs.toInt();
                }
                
                effectManager.SetGlobalColor(lastManualColor); 
                effectManager.SetInterval(0);

            break;
            ...
        }
        
    }

Yeah, its pretty ugly and does not conform to code formatting standards. But, it does not crash when I press a button that is not defined. To make it so that a user can change button settings via a settings file or web interface, it had to get more complicated. It will crash when running too many LEDs off of a MacBook Air usb port and after setting the global color to white or close enough to white to max out the available amperage. Took me hours to figure out that random crash, but that's another story.

@robertlipe
Copy link
Contributor

robertlipe commented Sep 29, 2023 via email

@revision29
Copy link
Contributor

I like them idea. Can you make this a "real" pull request, please? If you don't mind adding me as a collaborator, I can quickly help you get that past 1990-era C, for example. Commenting on text in email is just error prone.

@robertlipe To have a "real" pull request I first need to get a proper git setup going. I simply pulled the files to my computer and have been hacking at them locally. Not ideal, especially considering the number of changes that Dave has been committing in GitHub. But, I am not even a contributor to the project (yet). I'm just throwing out some ideas to get the code to work like I want and to hopefully add something of value.

The code needs to be cleaned up significantly before it gets added to the project. For sharing in this thread, I copied the code to a markdown editor to remove a bunch of garbage from debugging and make it readable in the browser. The raw code on my computer is too embarrassing to show. I also want to get the basic functionality of each button working before it's ready. I'm working with a 44 key remote which has more function than Dave's 24 button version in the original code, but there is some overlap. I really want to get the brightness functions working because I need that for real world projects in the works. It is far more a pain in the butt than I originally thought it would be. I also need to figure out some basic effects to add to buttons like Fade3, Fade7, etc. and integrate those without nuking the effects added to effects manager at boot.

I have to laugh at your 1990 era C comment. Nothing says 1990 coding like coordinating programming efforts over email and online chat groups. We've gone from listservs and aim/irc to GitHub managed emails and Discord.

Anyway, once I get the GitHub desktop client setup, fresh copy of the code pulled down I will port over my code to that codebase and move on from there. That will simplify development and feedback.

@rbergen
Copy link
Collaborator

rbergen commented Sep 29, 2023

@revision29 I really appreciate the effort you're putting into going through the learning curve that comes with this project's tech stack (which I appreciate can be very steep), and I think refactoring the setup of the remote control logic/code can be a great step forward concerning the maturity of that particular control interface.

However, as I've mentioned before (in this comment, to be exact) there is one thing I really don't see happening, and that is connecting the web UI to the IR remote control logic. The basic reason is that they are two completely different user interfaces to the same back-end, that largely being the effect manager and the effects it manages.

The IR remote is a "matrix of buttons" without feedback, input fields, sliders, checkboxes, "are you sure?" dialogs, etc. The web UI is everything that I just said the IR remote is not, and more. They're just two completely different beasts that can be used to manage the same back-end. As such, they should be reasoned about separately, considering the specific capabilities and limitations of each.
Reasoning from a different angle, the webserver that publishes the web UI should be able to run on devices without support for the IR remote, and vice versa. We already have quite a few dependencies within this project that one could scratch one's head about, and some efforts are now being made to disentangle those. I really don't think we should introduce a new one.

So again, I'm all for refactoring, modularizing and enriching our IR remote code structure and capability. But let's do that for the merits of the IR remote interface itself. The Web UI has and will have its own development and improvement tract.

If this comes across a bit strong then that's really just because I don't want anyone here to waste brain cycles or lines of code by considering the complexities of something that is and should remain entirely unrelated to the topic at hand.

As always, I'm happy to elaborate if what I just said raises questions or concerns.

@revision29
Copy link
Contributor

@rbergen Thanks for providing direction. I certainly don't want to waste my time on features that won't be used or will cause problems down the line. I don't want to cause collisions between what the IR remote does and the web server. The only reason why I am thinking about the web server is that when a device has a web interface people usually assume some settings can be adjusted from that interface. If the web server were to be expanded to include that functionality, it would be relatively trivial to also allow it to adjust button functionality if the buttons are defined in a user settings file. For the purpose of my current refactoring, it does not matter if the web server is ever involved. I have written my code so that it can be tweaked to interact with a user settings file if that were introduced into the software at a future date.

But, I will more closely read the comment you cited as well as the surrounding context. I have not read many of the discussions outside of this current thread. It's a firehose of information as it is.

@robertlipe
Copy link
Contributor

robertlipe commented Sep 29, 2023 via email

@rbergen
Copy link
Collaborator

rbergen commented Sep 30, 2023 •
edited

The only reason why I am thinking about the web server is that when a device has a web interface people usually assume some settings can be adjusted from that interface. If the web server were to be expanded to include that functionality, it would be relatively trivial to also allow it to adjust button functionality if the buttons are defined in a user settings file. For the purpose of my current refactoring, it does not matter if the web server is ever involved. I have written my code so that it can be tweaked to interact with a user settings file if that were introduced into the software at a future date.

@revision29 Thank you for clarifying that, that's very helpful. I'm happy to hear you're not approaching this from the perspective that the web UI should "push IR remote control buttons" in the sense of back-end logic - that in particular is what I'm objecting to.

We can certainly consider making the button assignments configurable "at runtime" from the perspective of the IR remote, and consider how to "logically structure" the storage of this in some sort of flat file - in the end that's how any configuration of this nature would have to "land" on this type of device anyway. We probably should also consider how to make that configuration "externally digestible", acknowledging that the IR remote is probably not the user interface that will be used to change its own configuration.

And as it stands, the web UI may well be the only viable front-end to modify this configuration. I still think we should isolate the two in our thinking, initially. Let's make the configurability setup work from the IR remote perspective first, possibly considering a future "abstract" external source of configuration capability when it comes to structure, naming and internal C++ interface. After that, we can worry about the consumption of and manipulation of this configuration - either via the web UI or, who knows, other channels.

@robertlipe
Copy link
Contributor

robertlipe commented Sep 30, 2023 via email

@rbergen
Copy link
Collaborator

rbergen commented Sep 30, 2023

Telnet. Like God, Herself, intended. 😁

You know, I actually think that would be pretty darned cool! :)

@revision29
Copy link
Contributor

@robertlipe

I have probably 15-20 IR colored remotes that I'm willing to photograph and integrate.

The problem is not so much the button layout, as we need to compile a list of special buttons they has such as flash, strobe, eject, etc. I want to replicate each of those labels as an entry in the enum to allow decent amount of customization. I am currently using a 44 key remote that came with a less capable LED light set. I also have a DVD remote I intend to re-purpose. It's form the dame DVD player I sourced the IR diode from. My goal is maximum customization to admittedly ridiculous levels.

We need a table that describes a physical remote, maybe we store key positions or labels or a link to a picture or something. Honestly, the majority of my basket has NO identifying information.

My design intention is to hard code as little as possible into the logic. My assumption is that a person will need to take the time to capture the codes for each button and then customize form there. My opinion is that it is needlessly complex to try and pre-populate a table with all of this data. Maybe a future feature would be to simplify the keycode acquisition process. Or we can provide some instruction on how they can capture the keycodes. This project requires a minimum technical capability and capturing keycodes should not be a surmountable task. If the person is soldering and tweaking code, they should be able to follow a guide to get the codes.

Does each key need a position in x,y and a label?

Presently as I have written the end user puts the button definitions in the userremote.cpp file. It's more or less function as the place where they adjust those settings. As I have done, they can layout those definitions in rows to make more visual sense. Coding in remote positions introduces unnecessary complexity. Maybe if there is ever a user settings file we can specify X columns by Y rows, but that would only make sense if you are trying to draw a visual preview in a web interface. The label is not needed for functionality but is more of a convenience to help a person find the button definition as it corresponds to a theoretical label on the remote.

Now, you just make a single call into effectManager.doAction(action).

The remote functionality needs to be distinct from the effects manager code. Things like FADE7, FLASH3, etc. will require building an effect stack and sending that to the effects manager. The reason I have it coded as is, is so that a person can go to the switch in the remotecontrol.cpp and trigger whatever action they want to customize. I do have an arguments string in the button class in case we need to pass arguments to effects manager. We don't want people digging through the effects manager code to try and customize button / enum actions.

"RemoteButton" is just a struct member. It's data. It does nothing by itself and doesn't seem to make sense as a class. You seem to be very class-happy. :-)

Fair point. I much prefer object oriented programming. I'd rather reference button.name instead of button[0]. It's much more readable for someone trying to make sense of the code and makes it so I don't have to remember all of the index numbers when sleep deprived.

There's code elsewhere in the project for making colors from hex.

Yes. It accepts a uint32_t RGB color code. My button definition code is pulling the color code from the arguments property of the button object. That property is a text string to maximize functionality. My solution of converting the text string to 3 separate color values is the best I could come up with. Since I am new to all of this I don't know what functions are more or less optimal. All I know is that it works. I would prefer being able to directly convert the hex text string to uint32_t, but I found no such solution googling. I mean, you can convert the string to an integer but it will not be the kind of integers you are looking for.

YOu can probably replace "this->" with "" and it all work the same.

I spent too much time with ecmascript / php back in the day. Will fix. Thanks for helping me overcome bad noob practices.

Even if buttons is a std:vector instead of a std::array or a plain ole array, you can still struct init it.

I can't remember why I settled on this. I think it has to do with vectors are easier to build on the fly rather than having to declare the whole thing at once. I want the code to be agnostic about the number of buttons a person is utilizing and just build a collection of what buttons they want to define. From my googling it is much easier to add / remove from a vector than it is appending an array.

Your "random" crash isn't random. It's power supply overload. I'm honestly shocked the MBA doesn't have current overload protection. By design (I attended USB design meetings pre-ratification) you were supposed to be able to twist all the wires together and not cause a harmful short.

It seemed random at the time. The problem is I changed how board connected to the computer. In my previous method, the USB hub previously used requested additional power from the USB port (as allowed by the USB-C spec). When I switched to a kludgy cable only solution, the USB port defaulted to a lower allowed wattage, probably whatever USB2 defines. The power negotiation process did not come to mind at all when I switched my connectivity solution. I had erroniosly assumed that when I was plugging in my power adapter it was switching to use that automatically and not drawing power from USB. This situation was compounded by the fact that in my code, I added some debug code in a few places to track when things were triggering. I made some mistakes and was passing the wrong variable types into the debugger. This was when I was trying to figure out the RGB hex string conversion. The code compiled, but when the debugger received the wrong variable type it crashed the board and rebooted just like when it browned out. I had crashes coming from like 3 sources. I got the code problems solved and it was still crashing for "no good reason" . It was at 1:50am when I noticed the brownout error being printed to the console before the board crashed and it clicked what was happening. Then I figured out the proper power connection sequence and all is good. But, part of this is that I had forgotten that I added a color fill effect of pale green after the standard demo rainbow effect. So, at 30 seconds it was guaranteed to crash. But, also I was triggering crashes when I pressed remote buttons before the 30 seconds. It was a bad mix of hardware variables, trying to hack out text conversion functions, frustration, and a late night. I thought it was pretty funny once I figure out what was going on. I had gone through the trouble of disabling all of my code and putting the original code back in place and it was a hardware issue.

TLDR; I got a good laugh over a bad combination of bad code and hardware changes.

@robertlipe
Copy link
Contributor

robertlipe commented Sep 30, 2023 via email

@revision29
Copy link
Contributor

Follow up: on a positive note, I got brightness up and down to work with the brightness buttons. I want the brightness level to apply across all effects, not just the ones triggered by the remote. I want a global brightness level that the effects manager reads and applies proportionately to all affects. Partly for the remote functionality, but primarily because in most of my use cases I want to boot at less than full brightness. I can see that will be a nightmare to implement.

@robertlipe
Copy link
Contributor

robertlipe commented Oct 1, 2023 via email

@rbergen
Copy link
Collaborator

rbergen commented Oct 1, 2023

I got brightness up and down to work with the brightness buttons

Great news, well done!

I can see that will be a nightmare to implement

Why would that be? The setting could be held by the DeviceConfig class, with the benefit it could then also be controlled via the web UI. The only thing we'd have to add is a mechanism to inform EffectManager that the setting changed. I'd probably generalize that by creating and using some sort of event listener interface - or see if the standard library already offers something like that.
It's work, but I'd say quite manageable.

Or maybe I misunderstand your remark?

@rbergen
Copy link
Collaborator

rbergen commented Oct 1, 2023

FWIW, I've considered adding a feature to ramp brightness from 20% -> selection on bootup

Yeah, that's a different animal.

@robertlipe
Copy link
Contributor

robertlipe commented Oct 1, 2023 via email

@rbergen
Copy link
Collaborator

rbergen commented Oct 1, 2023 •
edited

They're not a terribly difficult design pattern to implement.

Turning that into a prophecy of sorts, I think I'd keep it simple at a conceptual level (read: naive) in this case. I'd create an interface class with one "pure virtual" function that's called something like OnSettingChanged, which accepts the sender (there is some decision making to be done on how to do this exactly, which might make the pure virtual a template instead) and the changed setting name as its only parameters. That means the interface class can be used for both device and effect settings, and is usable for observers that observe both.
Observer and the observed would have to have a gentleman's agreement that the called functions behave and return quickly, but I think this use case is concrete and simple enough for that to suffice.

Independent of this work, if I implemented the "power up fade in slowly over N seconds" part, Rutger, would you be interested in handling the UI part of setting N? Or we just declare that N is 2 or 3 or something and forget the UI, in which case your half could be done now.

I'd actually choose a sensible N for the user, but make the power-up fade-in a toggle setting; i.e. let the user decide to enable it or not. I'm happy to take care of the DeviceConfig and CWebServer changes to add this when the rest is in place.

Do we prever g_values->Brightness (MatrixScaledBrightness?) over LedMatrixGfx:SetBrightness or FastLED::setBrightness or is there yet some other, more authoritative, knob?

Apparently like you, I got lost in the many seemingly similar place there already are to "put something about brightness", and decided to not touch that part until now, in part for that very reason. Taking a few steps back I would say that if we want this to be a device-wide setting then sticking it in DeviceConfig seems to make sense, but I also note we already have one exception (the effect interval stored in EffectManager). So, maybe we should figure out what location makes most sense from the perspective of the logic using it, and take it from there.

Sanitizing this part of the project would definitely feel like an improvement to me, although I have to say that my knowledge of that corner of NDS is not enough to be able to say how much we could actually simplify or unify without breaking stuff.

@revision29
Copy link
Contributor

If there is a method to adjust brightness, I have tried it. g_values->Brightness is hard coded to 255 in code and just seems to be used for outputting stats to the console or built in OLED screen. It's more or less a dummy value, currently. FastLED::setBrightness seems obvious, but the current stack for drawing the led pixels completely bypasses whatever this method does. In a normal dataflow you set FastLED.setBrightness() then FastLED.show() or something like that. Bottom line, it does not work with this implementation.

The magic sauce is in a CRGB object. It has a method "maximizeBrightness" which takes a brightness integer. You can maximize between 0 (which effectively sets the color black, making it impossible to bring up the brightness) or 255 which will make the color as bright as possible. In my code I step by 16. The nightmare scenario is changing the brightness on the color sent to each led every time it is sent. For effects where each led might conceivably have a different brightness level (twinkle, fire, etc.), the desired brightness would have to be multiplied by the fractional global brightness. This is the rabbit hole I am heading down currently. I just need to find where in code the color is specified for each pixel and inject some code there to set the brightness based on g_values->Brightness . This is probably similar to what a vanilla implementation of FastLED does behind the scenes. The beauty of this approach is that it will change the brightness on the next frame draw without the need for a watcher since it would just pull the brightness level from g_values->Brightness.

The short version is we need to adjust the color of each pixel using it's maximizeBrightness method just before it is sent out to be drawn.

@robertlipe
Copy link
Contributor

robertlipe commented Oct 1, 2023 via email

@revision29
Copy link
Contributor

revision29 commented Oct 5, 2023 •
edited

Status update: universal brightness control achievement unlocked. I am utilizing g_Values.Brightness as the universal brightness level. I don't know if this is being stored on device along with the effects manager settings and persists across reboots, but that is something to figure out later. Theoretically a person can just hard code the brightness level to their liking and never touch it again with IR remote or any other mechanism. In other words they can build the code to their specific use case / desired brightness and deploy without ever having to interact with it again. The code to handle all of this is embarrassingly trivial. Most of the work was following the logic and trying to find more detailed FastLED documentation.

In ledstripgfx.cpp I changed the lines:

for (int i = 0; i < NUM_CHANNELS; i++)
FastLED[i].setLeds(effectManager.g(i)->leds, pixelsDrawn);

to

for (int i = 0; i < NUM_CHANNELS; i++) { //changed to the more complex curly brackets format
FastLED[i].setLeds(effectManager.g(i)->leds, pixelsDrawn);//Left untouched
for (int j = 0; j < FastLED[i].size(); j++){ //Added this for loop to adjust the brightness of the pixel in FastLED. This leaves the original pixel in effectManager untouched.
FastLED[i][j].fadeLightBy(255-g_Values.Brightness); // we fade by the inverse of the brightness level.
}
}

The remote control code just adjusts the global brightness up and down giving about 20 levels of adjustment. I have not looked closely but assume the same tweak will be needed in the ledmatrixgfx.cpp file.

I ended up going with the fadeLightBy because it fades light proportionately instead of setting to a static brightness level as my previous experimental implementation did on a solid color fill. This means that when a person writes an effect where the LEDs might have different brightness levels (like the fire effect) all LEDs will be dimmed the same percentage. If the brightness is 128 each LED will output at about 50% the luminosity set through the effect. If brightness is 192, the brightness will be at about 75%.

Tip: is first declared in values.h and is subsequently set to 255 in ledstripgfx.h. This seems like a redundancy. But changing the value to both of these should set the brightness at boot.

Note: it seems like the matrix code already implements brightness controls. I will build a small test matrix to see if my remote code will also trigger brightness changes in the matrix.

@rbergen
Copy link
Collaborator

rbergen commented Oct 5, 2023

Nice, well done!

I don't know if this is being stored on device along with the effects manager settings and persists across reboots, but that is something to figure out later.

This I can answer straight away: it isn't. In short, for it to be persisted and loaded on boot it would have to somehow end up as a device setting in the DeviceConfig class.

Note: it seems like the matrix code already implements brightness controls. I will build a small test matrix to see if my remote code will also trigger brightness changes in the matrix.

Please note that LEDMatrixGFX is only used for HUB75 matrices. WS1812x matrices are driven using the same LEDStripGFX class that actual strips are.

@davepl
Copy link
Contributor Author

davepl commented Oct 5, 2023 via email

@rbergen
Copy link
Collaborator

rbergen commented Oct 5, 2023

Rutger, do we persist last brightness? We should!

@davepl No, we don't. As I commented about 15 minutes ago, for that the brightness would have to be "promoted" to a device setting in DeviceConfig. It would have the added benefit that the brightness can also be changed/set using the Web UI.
With that brightness device setting in place:

  • the "setter" for the brightness setting would also need to update g_Values.Brightness
  • all code that now changes the "global brightness" by modifying g_Values.Brightness would need to change the new device setting instead.

Nothing too complicated overall. I'm sure I can put a PR for this together, this coming weekend.

@revision29
Copy link
Contributor

2 things as a follow up:
First: I need to get things ready for making commits to the remote control code. My local repository was way out of sync with the main branch. Getting that straightened out was a pain. I am getting my environment prepared for the eventual task of pushing changes / making pull requests to the remote code. Using the desktop client, I made a pull request for a small, yet important change to effectmanager.h . This is a test to make sure that my setup is setup properly and also correct an annoying bug in the said file. I am still a few weeks out before I will be ready to push the remote code.

Second: Also, I am going to propose putting the main brightness up and down logic into the effect manager instead of the remote. This will allow a person to trigger the adjustment from other interfaces that interact with the effect manger. If a person has a physical push button wired or repurposes a button on the board it can be coded to trigger the brightness change instead of having to interact with the remote control code. I propose three methods: SetBrightness to set a definite level. IncreaseBrightness to increase by 13/255 or about 5%, and DecreaseBrightness to decrease by 13/255. This will be an easy change and will interact with g_Values.Brightness unless / until a different variable is used. (But, to be honest g_Values seems like a decent place to keep it since a person can just set the initial value to be whatever they want at compile time and use their adjustment method to change.) I will issue a pull request soon if this is a good idea. That is assuming my I have everything configured properly to trigger pull requests.

@rbergen
Copy link
Collaborator

rbergen commented Oct 6, 2023

Second: Also, I am going to propose putting the main brightness up and down logic into the effect manager instead of the remote.

@revision29 We will move the main brightness setting (and any logic for changing it around it) not to EffectManager, but to DeviceConfig, as I mentioned in my previous comment. I'll take care of implementing that setting (as well as replicating it to g_Values.Brightness and exposing it to the web UI) this coming weekend.
To be honest, I'm not fully convinced that moving the up/down logic away from the remote makes sense, unless and until there really is a second interface that uses the exact same change intervals that the remote does. That said, I don't have that strong an opinion about it either.

@rbergen
Copy link
Collaborator

rbergen commented Oct 6, 2023

@revision29 So the shift of g_Values.Brightness to DeviceConfig is now in the brightness-setting branch in my fork. In the end I decided to remove g_Values.Brightness altogether, because the number of places referencing it was actually rather limited - all of those now read and write brightness in DeviceConfig.

I'll run a final test on PR #451 that I opened a few days ago, and merge that if that still works. I'll then open a new PR for the brightness setting change.

@revision29
Copy link
Contributor

revision29 commented Oct 6, 2023 via email

@rbergen
Copy link
Collaborator

rbergen commented Oct 6, 2023 •
edited

@revision29 The PR that will make main brightness a persisted device setting is #454. I did not implement methods to increase or decrease the value (by a value passed or not), but you're free to incorporate that into your future PR. If it's in there I'll consider it again and actually make up my mind about it.

I looked for your global color code PR in this repo, in Dave's fork and in your own fork, but I've been unable to find it in any of those places. I'm sorry to say that means that I can't think of where it went, if anywhere.

@revision29
Copy link
Contributor

@rbergen does the code in the current pull request for 454 relate to someone's particular remote control? If so, I will want to replicate that functionality when I push changes to the remote control code in a few weeks.

I also added a code review for the pull request for 454 to suggest a change to make the brightness level actually apply to pixels drawn with the led strip effects. All of the work done for the brightness variables mean nothing if the code does not actually apply the brightness level. Once that tweak is done and the rest of that pull request is applied, the remote brightness controls should actually work for led strip effects. Testing will need to be done with matrices to see if any changes need to be made there.

Lastly, I found the right place to make a pull request and have done so for effectmanager.h.

@revision29
Copy link
Contributor

It is so nice to power cycle a device and the LEDS stay at the low brightness level I set them to with the remote. Thank you @rbergen on getting the brightness code into device config.

@rbergen
Copy link
Collaborator

rbergen commented Oct 7, 2023 •
edited

@revision29

does the code in the current pull request for 454 relate to someone's particular remote control?

It leaves all of the existing remote control logic, i.e. as it is in the current codebase, untouched. So, the remote control logic in #454 operates exactly the same as the logic in the current main branch does.

I also added a code review for the pull request for 454 to suggest a change to make the brightness level actually apply to pixels drawn with the led strip effects. All of the work done for the brightness variables mean nothing if the code does not actually apply the brightness level. Once that tweak is done and the rest of that pull request is applied, the remote brightness controls should actually work for led strip effects.

The suggestion is fair enough, but I won't apply it in this PR. The goal of #454 is making brightness a persisted property and nothing else. The change you suggest I can't actually even apply responsibly, because I personally can't test it.

I think the approach to take here is one in two stages:

  1. After review, we merge Make brightness a device setting #454 to arrange brightness being persisted.
  2. A second PR is opened after that by someone else (possibly you) to make changes to the remote control logic that are deemed necessary for brightness to apply to LED strips and have been tested to work.

Testing will need to be done with matrices to see if any changes need to be made there.

...and that is something I can possibly help with, before or when the second PR I just mentioned is opened.

It is so nice to power cycle a device and the LEDS stay at the low brightness level I set them to with the remote. Thank you rbergen on getting the brightness code into device config.

That's nice to hear! You're welcome!

@revision29
Copy link
Contributor

@rbergen Once the merge is done I would be glad to do the pull request to apply the brightness level to the output.

@rbergen
Copy link
Collaborator

rbergen commented Oct 8, 2023

I'll run some additional tests myself, today and then merge the PR if they succeed. I'll comment here when that happens.

@rbergen
Copy link
Collaborator

rbergen commented Oct 8, 2023

I just merged #454.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@davepl davepl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@revision29 @davepl @robertlipe @rbergen @smiliea