esp8266_milight_hub icon indicating copy to clipboard operation
esp8266_milight_hub copied to clipboard

added support for Casalux cct panel

Open aeinstein opened this issue 5 years ago • 6 comments

added support for the Casalux cct panel sold from Aldi in Germany. Please merge only commit # db77d0e. This is my first pull request, and I can't find out how to only select this commit for pull, sorry

aeinstein avatar Jan 04 '20 21:01 aeinstein

@aeinstein: In case you didn't find out yet: Normally you would make a branch in your repository to do your development work and then just merge the wanted changes to your master and offer that for the pull request.

If you already changed master you can do it the other way around and make a branch with only the wanted changes and offer that for PR.

Merging a single commit from PR like this is not really possible.

Cheers!

normen avatar Feb 13 '20 12:02 normen

Totally missed this, sorry @aeinstein.

Appreciate you adding this, and impressed that you were able to make this deep of a change unaided.

Couple of high level concerns I have with merging this --

  • Correct me if I'm wrong, but this seems like a niche device. Adding new remote types and especially radio configs introduces overhead. So obviously I'm hesitant to do that unless a lot of people use the device.
  • Seems like there are some extraneous changes here. Namely changing the RF24Channel enum to uint8_t (I prefer enums), some references to an NLG remote type that doesn't look to be fully supported in your changes, and some changes to the default settings.

Let me know what you think.

sidoh avatar Feb 14 '20 04:02 sidoh

@sidoh : I totally agree with you, nobody wants more overhead. I think about some checkboxes in the config tab for enable/disable the types of radios (channels, syncwords) that are used by the user. (I only have casalux and NLG)

Also I dont wanted the other changes (free channel input) to be merged, this was only for understanding and testing purposes. Sorry for my wrong usage of github, I used to act with my own git server. (blame on me). I suggest to cancel the merge request and I make a new branch for the casalux impl. and one more for the changes of radio config.

The NLG impl. is not yet ready and I struggle with it. Cant find the Checksum Algo. Perhaps you can help, if I give you the sniffed raw data?

aeinstein avatar Feb 14 '20 09:02 aeinstein

@aeinstein

I have a casalux panel, too. But I cannot compile your version successful. I get several error for nodemcuv2, example:

fatal: not a git repository (or any of the parent directories): .git

Can you help me? Compiling of the normal version runs fine

Eisenhauer1987 avatar Dec 09 '20 14:12 Eisenhauer1987

@aeinstein ... I am a general user of this software and I learnt a lot from your PR (as to how I can go about submitting fixes and features.)

If I may, I think for this PR to get merged it needs the following:

  1. Resend a fresh clean PR with only the changes that needs to be merged
  2. Have the commits be atomic and build up to the end state with relevant commit messages
  3. If anything is changed, accompany comments or documents explaining why.

Lastly, various device type support can be made an early stage config read and then flagging via and enum to avoid commuting unneeded code-paths and not have overheads.

shamasis avatar Feb 22 '21 07:02 shamasis

I have a NLG Panel and cant get to sniff the codes

gtawelt avatar Sep 22 '22 14:09 gtawelt

I have a NLG Panel and cant get to sniff the codes This module don't understand NLG, cause I dont understand the checksum. I suffered to find the right algorhythm, sorry. I will try it again, when I rebuild my smarthome

aeinstein avatar Dec 15 '22 15:12 aeinstein