search

rogerdahl / m5stickc-idf

ESP-IDF component to work with M5StickC without Arduino
MIT License
12 stars 5 forks source link

Feature power init #6

Open teuteuguy opened 4 years ago

teuteuguy commented 4 years ago

This feature adds ALL the previous pull requests and changes the init function of the AXP182 to set it up as per the "official" M5StickC repo.

pablobacho commented 4 years ago

I would like to discuss further these changes. Even though the init function is cleaner this way, this PR conflicts with the option of customizing the AXP192 registers at the beginning.

For example, with the default register settings the display flickers and shows a brief screenshot of the last image it had before turning off. It just looks bad and cheap. It should turn on clean. Keeping the backlight off until the display has been cleared gives you a smooth power up look. Also the backlight flickers if you turn it on at max power and then immediately lower it after init.

I do not want to stick to how the M5StickC Arduino library initializes registers, and I think that should be left up to the user to decide. That is why I created the m5power_config_t struct. Right now it just has the option of setting the screen initial values, but the idea is to add more settings later as needed.

If you have to process the struct to change the register value array and then process it in a loop, the init function ends up being just as long as it was before the changes + the added content of the array. I agree that it is a repetitive task that looks better in a loop, but not all registers will be processed exactly the same way.

An alternative would be, since power normally will be initialized only once at the very beginning, using menuconfig for that customization and removing the m5power_config_t struct entirely so we can use your init implementation. I think that would be good.

What do you think?

teuteuguy commented 4 years ago

Very good comments. I first saw differences in your init vs the one that M5Stack have posted, and I assumed that the M5Stack one was "the truth". I do feel writing it as they have (indepentently to the setup itself), is clearer, and easier to change if someone wants.

Thought, why not make the table a default, and open up the user to creating their own and passing it to the init function? Going to toy with the idea. I do like your m5power_config_t struct, however adding ALL the options in there ... ouch.

Going to play with it

teuteuguy commented 4 years ago

Forgot. I really like the defaults being in the menuconfig ! Going to try that actually.

teuteuguy commented 4 years ago

Pablo, I've worked on the following branch, for this feature. I moved the defaults to Kconfig so that it can be menuconfig'd. https://github.com/teuteuguy/m5stickc-idf/tree/feature/improved-power-init Let me know what you think?

pablobacho commented 4 years ago

I haven't said anything because I am short of time these days. But I will check it out and get back to this.

teuteuguy commented 4 years ago

Sure. I’ve gone ahead with the change on my end for workshops I run on amazon FreeRTOS. (https://github.com/teuteuguy/amazon-freertos-m5stickc-workshop/).

Your recommendation to use the menu config I find very good so I implemented it. Keen to get your feedback and also, your challenging of the setup default params and why.