openshwprojects / OpenBK7231T_App

Open source firmware (Tasmota/Esphome replacement) for BK7231T, BK7231N, BL2028N, T34, XR809, W800/W801, W600/W601, BL602 and LN882H
https://openbekeniot.github.io/webapp/devicesList.html
1.45k stars 269 forks source link

Code quality sweep? #715

Open lmcd opened 1 year ago

lmcd commented 1 year ago

This project has been a life saver, as I was hoping to use Tasmota, but just learned recently that all the new bulbs on the market carry this new chipset, so thanks for all your hard work.

I was hoping to maybe contribute and/or do a personal fork of the project.

Would it be possible to do a quick sweep of the project to cleanup and standardise everything, as I see a lot of placeholder/commented out code, inconsistent variable names, uncommented files, a lot of things prefixed with 'new' etc. Would also be useful in some instances to have abbreviations described at the top of files (e.g. I don't know what HAL means), and some explanation of whatever the Windows part is doing.

Would also love some kind of way of modularising this so I can have a reduced build that only does bulb stuff over HTTP with everything else gone - but I'm not sure how exactly I'd get started with that in C.

openshwprojects commented 1 year ago

Hello, thank you, yes, it's planned and it's a good idea.

We really need to fix coding style and stuff.

During past days, I've updated docs so they now really exist and are autogenerated from source code files and json files, and the JSONs can be also used to create HTML docs page.

In the past months, I have also prepared a "self test system", which is basically like a unit testing of obk firmware that runs on windows. This will be helpful in the reorganization.

But one step at time. If you have any direct suggestions what to do, let me know.

Regarding reducing build - I don't know a sensible way to do this, but we already have online and docker builds so there is no problem....

What would you like to contribute with?

I can write you a brief roadmap of changes I have planned.

lmcd commented 1 year ago

We really need to fix coding style and stuff.

Is there a particular C code style you think we should adopt?

Regarding reducing build - I don't know a sensible way to do this, but we already have online and docker builds so there is no problem....

I think for me it's more of a case of reducing surface area of the codebase, to make it easier to understand. Having lots of different unused features, with references to them leaking out all over the codebase adds a lot of mental overhead when trying to understand things.

What would you like to contribute with?

I have an idea I would like to explore to very accurately sync a series of lights within a few milliseconds, and then send a pre-determined sequence of changes with timings over UDP to all lights at once. I'd like to do this, as I previously had issues with Tasmota, where occasionally a light wouldn't change in time due to network conditions/lag.

Also, the UI is kinda awful, and would love to come up with something super slick and nice.

ekobres commented 1 year ago

I have an idea I would like to explore to very accurately sync a series of lights within a few milliseconds, and then send a pre-determined sequence of changes with timings over UDP to all lights at once. I'd like to do this, as I previously had issues with Tasmota, where occasionally a light wouldn't change in time due to network conditions/lag.

That would be super cool.

Also, the UI is kinda awful, and would love to come up with something super slick and nice.

Yeah, it's a big project, but it looks like the project could benefit if all of the hand-coded C that deals with http were to focus only instead on implementing a well-designed and documented REST API, and the whole UI could be converted to a nice JS application stored on the filesystem (or hosted on GitHub.io as today with the web app.)

It looks like there was a move in that direction - so now we have both a native http interface as well as a web app with overlapping functionality that is out of sync.

This is actually a tremendously valuable project with a ton of great work behind it from @openshwprojects - but as you say it could use some love and attention from an overall architecture and design standpoint to make it more user and contributor friendly.

lmcd commented 1 year ago

That would be super cool.

I detailed this here: https://github.com/arendst/Tasmota/issues/8305 This would guarantee synchronisation regardless of network conditions, so you could reliably sequence a set of lights to music or something without worrying about it screwing up

lmcd commented 1 year ago

Also, the UI is kinda awful, and would love to come up with something super slick and nice.

Yeah, it's a big project, but it looks like the project could benefit if all of the hand-coded C that deals with http were to focus only instead on implementing a well-designed and documented REST API, and the whole UI could be converted to a nice JS application stored on the filesystem (or hosted on GitHub.io as today with the web app.)

Yeah, I'd appreciate a cleanly designed REST api, and also a pub/sub WebSocket endpoint where I can subscribe to events, and receive them in realtime on the webpage, to negate the need for constant polling or page refreshes.

openshwprojects commented 1 year ago

I have a 90% finished step by step guide/tutorial how to use REST API to create full page control for any device, it's possible even with our Tasmota JSON emulation. Even Tasmota Control from Google Play works. Futhermore, it's possible to host files on LittleFS so you can create custom page.

You can also create custom Web Application as well.

The thing is, we can't just totally remove native HTTP interface, because otherwise you'd need a network connection to fetch Web App from network and also we need native HTTP for safe mode.

@lmcd isn't it what the DDP protocol with xLights is for? We already support DDP.

I will think about that coding style.

The plan for Web App is to be an extended version of control panel, and the Native HTTP will stay basic.

I might remove some more advanced things from HTTP in the future, like the long flag descriptions that are taking space in flash memory, but I think it's worth to keep at least basic native panel functionality.

lmcd commented 1 year ago

@lmcd isn't it what the DDP protocol with xLights is for? We already support DDP.

Wow, it already exists. Awesome!

lmcd commented 1 year ago

It looks like DDP has extremely limited support and doesn't do any of the syncing/timing yet - just the basic ability to send a message to a bulb to set it's rgb - unless I'm missing something.

openshwprojects commented 1 year ago

Yes, it's very basic, would you like to help extending it?