search

netmindz / balboa_GL_ML_spa_control

Control protocol between GL2000 controller and ML series compatibile top panel
17 stars 8 forks source link

Pump speed states #35

Closed caldog20 closed 1 year ago

caldog20 commented 1 year ago

I hope you don't mind, I did run clang-format to get a standard formatting since some of the formatting was different across the codebase.

I made a small adjustment to support having 2 dual speed pumps. I wanted to open so others could verify.

netmindz commented 1 year ago

Generally better to have single-purpose PRs, but I'll let you off this time ;)

I only have a single speed pumps, code looks sane, so I'm tempted to just merge

caldog20 commented 1 year ago

Generally better to have single-purpose PRs, but I'll let you off this time ;)

I only have a single speed pumps, code looks sane, so I'm tempted to just merge

Sorry, that's why I split it into 2 different commits. :P

If you only have single speed pumps technically you should never get those other values so I am not sure it would affect you. But if you could test that would be awesome!

netmindz commented 1 year ago

Yeah I appreciated the different commits, do need to merge this quickly though to reduce risks of conflicts.

@tmjo Can you test these changes? Merge if works as expected

tmjo commented 1 year ago

Will see if I can test this today or tomorrow.

netmindz commented 1 year ago

Any luck testing @tmjo ?

tmjo commented 1 year ago

Any luck testing @tmjo ?

No, sorry. It's been raining sheep and cows here lately, so I wasn't able to test yet. Forecast looking better, so hope to do it soon.

netmindz commented 1 year ago

I find people in the UK who don't own a tub think they are best when the weather is warm, I always say that if anything when the weather is cold is actually better, that any day is a tub day - unless it's raining ;)

tmjo commented 1 year ago

I find people in the UK who don't own a tub think they are best when the weather is warm, I always say that if anything when the weather is cold is actually better, that any day is a tub day - unless it's raining ;)

True, true. Even light rain can be nice, but not while debugging with a computer :) How are you guys testing, do you have the serial cable going inside, or a long usb-cable? Or just staying outside as well? I was looking at some 10m usb-extensions, but not sure if it will work.

tmjo commented 1 year ago

Guys,

I've been fooling myself it seems. My Pump1 is two-speed, but my other pump is probably not Pump2. It is labelled Aux on my panel, and from my old notes I found out it gives value 'c' in the location before pump (result.substring(12, 13). I'll double check, but that's what I found in some older loggings. I haven't bother much until now because my aux pump is broken anyways.

@caldog20; Could you do a double check of the values? I have a strong feeling it is all bitcoded, and it would make perfect sense except for values '4' (would make sense if it was Pump1 Off, Pump2 Low) and '7' (which in my narrow minded logic would give pump1 both low and high at same time). Perhaps I am wrong, but it would be good to double check.

0       00000000    All off         ok
1       00000001    Pump1 Low       ok
2       00000010    Pump1 High      ok
4       00000100    ?           Would make sense if Pump1 Off, Pump2 Low
6       00000110    Pump1 High Pump2 Low    ok
7       00000111    Pump1 Off Pump2 Low Is this correct? If bitwise 11 would mean both low and high for pump1
8       00001000    Pump1 Off Pump2 High    ok
9       00001001    Pump1 Low, Pump2 High   ok
a       00001010    Pump1 High, Pump2 High  ok
caldog20 commented 1 year ago

I think the difference here is if you have a dedicated circulation pump? Mine doesn’t, so pump 1 is technically the circ pump and cannot be off if pump2 is on at any speed. My dip switch for circulation pump is disabled since I only have pump 1 and pump 2. Could this be it?

On Tue, Jul 4, 2023 at 4:03 PM tmjo @.***> wrote:

Guys,

I've been fooling myself it seems. My Pump1 is two-speed, but my other pump is probably not Pump2. It is labelled Aux on my panel, and from my old notes I found out it gives value 'c' in the location before pump (result.substring(12, 13). I'll double check, but that's what I found in some older loggings.

@caldog20 https://github.com/caldog20; Could you do a double check of the values? I have a strong feeling it is all bitcoded, and it would make perfect sense except for values '4' (would make sense if it was Pump1 Off, Pump2 Low) and '7' (which in my narrow minded logic would give pump1 both low and high at same time). Perhaps I am wrong, but it would be good to double check.

0 00000000 All off ok 1 00000001 Pump1 Low ok 2 00000010 Pump1 High ok 4 00000100 ? Would make sense if Pump1 Off, Pump2 Low 6 00000110 Pump1 High Pump2 Low ok 7 00000111 Pump1 Off Pump2 Low Is this correct? If bitwise 11 would mean both on and off for pump1 8 00001000 Pump1 Off Pump2 High ok 9 00001001 Pump1 Low, Pump2 High ok a 00001010 Pump1 High, Pump2 High ok

— Reply to this email directly, view it on GitHub https://github.com/netmindz/balboa_GL_ML_spa_control/pull/35#issuecomment-1620740867, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACR3MGD47INWMZO2RDROTGDXOSAK7ANCNFSM6AAAAAAZY66I6Y . You are receiving this because you were mentioned.Message ID: @.***>

caldog20 commented 1 year ago

I will still double check but right now this is the status I am using for my setup and it reads properly. But it could still be wrong so I will decode it again and double check.

tmjo commented 1 year ago

I think the difference here is if you have a dedicated circulation pump? Mine doesn’t, so pump 1 is technically the circ pump and cannot be off if pump2 is on at any speed. My dip switch for circulation pump is disabled since I only have pump 1 and pump 2. Could this be it?

Not exactly, because I have three pumps in total. The circulation pump is a dedicated and smaller pump, then I've got pump1 which is labelled Jet and has two speeds and performs the massage. In addition I've got this Aux which I've never been able to run but which is a pump motor too, and it seems to have only one speed (based on led going on/off when i toggle the button).

caldog20 commented 1 year ago

@tmjo Yeah you are correct. I am not sure where the "7" came from. This is infact bitcoded using 4 bits for the pump1/pump2 states. I will make the appropriate changes so you can test.

tmjo commented 1 year ago

That is awesome! I believe that is true for some of the other bytes as well such as status and heater/light byte at least. It could make the parsing easier and faster, IMHO we should at some point refactor to use less Strings and more raw bytes and just check byte for byte (without "string'ing" in between) and in some cases just check specific bits.

For instance I have noted:

1   00000001    STD     ok
2   00000010    ECO     ok
4   00000100    SLEEP       ok
9   00001001    STD CIRC    ok
a   00001010    STD CLEAN   Check? Should be ECO in Circ?
b   00001011    STD in ECO  ok? Should be STD in ECO Circ
c   00001100    SLEEP CIRC  ok
3   00000011    STD in ECO  ok

which leads me to believe that b4 may be circulation pump running. But not sure.

netmindz commented 1 year ago

Sounds like we are happy for me to merge this in.

As for string Vs btye Vs bit, would definitely welcome a PR tidying some of that up. Might well help the reboots I see sometimes that I think might be memory related. String was just easiest option when copying and pasting during initial experiments @tmjo

tmjo commented 1 year ago

Agreed, it was definitely a good way to start off with strings, made the whole process easier. What about opening a separate discussion on "Refactoring ideas" so that we could discuss which ways are best to go? I've got some ideas, but no need to spend time on something unless we all think it is a good idea.

netmindz commented 1 year ago

Can either @caldog20 or @tmjo open a clean PR with just the replacement of retries with the idle command change?