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.

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

Range extender clients list #17048

Merged
merged 6 commits into from
Nov 14, 2022
Merged

Range extender clients list #17048

merged 6 commits into from
Nov 14, 2022

Conversation

joba-1
Copy link
Contributor

@joba-1 joba-1 commented Nov 12, 2022

Description:

Related issue (if applicable): addresses #16047

Checklist:

  • The pull request is done against the latest development branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR and the code change compiles without warnings
  • The code change is tested and works with Tasmota core ESP8266 V.2.7.4.9
  • The code change is tested and works with Tasmota core ESP32 V.2.0.5
  • I accept the CLA.

NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 12, 2022

command output looks like this:

23:41:09.738 CMD: rgxclients
23:41:09.754 MQT: stat/nat2/RESULT = [{"Mac":"4c:eb:d6:77:f7:60","IPAddress":"192.168.4.2"},{"Mac":"ac:5f:3e:50:65:bb","IPAddress":"192.168.4.3"}]

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 12, 2022

@sfromis mentioned the output format is not friendly to tasmota rules processing.
Feel free to suggest changes before merging. it would be nice if it is easy to use the result to e.g. generate a list on the main web page

@sfromis
Copy link
Contributor

sfromis commented Nov 12, 2022

I'd say that the need for having the enhancement as a separate build-time option to be selected may not be that great, as I'm not expecting RGX to be to be added to "busy" builds where space is very tight. If people want the RGX feature, let them have the new command included too.

About the JSON format.... This format would be more like existing JSON payloads, with a consistent pattern for using them in rules and other places:
{"RGXClients":{"4c:eb:d6:77:f7:60":{"MAC":"4c:eb:d6:77:f7:60","IPAddress":"192.168.4.2"},"ac:5f:3e:50:65:bb":{"MAC":"ac:5f:3e:50:65:bb","IPAddress":"192.168.4.3"}}}
Sure, having the MAC (note uppercase-only for an abbreviation) twice is redundant, but it is more general, like the structure of other JSON messages from Tasmota. Of course, if something like host name is available (?), that would be nice as key name for the object for that device.

The RGXClients object level makes the JSON recognizable without already having to know that it is a command response from the new command.

A shorter format could be this, but less flexible in case of wanting to add other fields, like RSSI or hostname, to the payload:
{"RGXClients":{"4c:eb:d6:77:f7:60":"192.168.4.2","ac:5f:3e:50:65:bb":"192.168.4.3"}}

But again, I'm not trying to suggest a "need" for the more general format, more like pointing it out for consistency.

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 12, 2022

Thanks,

  • I like the {"RGXClients": part. I didn't notice the result doesn't give away where the payload comes from without that.
  • "Mac" comes from D_JSON_MAC, so I went with that. Otherwise I would have capitalized it for the same reason as you. Tend to keep it.
  • I don't like redundant, so I'd prefer {"RgxClients":{"4c:eb:d6:77:f7:60":{"IPAddress":"192.168.4.2"},"ac:5f:3e:50:65:bb":{"IPAddress":"192.168.4.3"}}}. This can be up to 10 entries, and maybe later more fields like hostname. Not sure how much space we have.
  • I prefer Rgx over RGX

@Jason2866
Copy link
Collaborator

@sillyfrog as author of the orig code, can you have a look?

@arendst
Copy link
Owner

arendst commented Nov 13, 2022

Let me have a look. I agree with @sfromis to have it included anyway.

@arendst
Copy link
Owner

arendst commented Nov 13, 2022

@joba-1 pls make the suggested changes (JSON like you suggested, no define needed, just include it).

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 13, 2022

ok

@sfromis
Copy link
Contributor

sfromis commented Nov 13, 2022

When it comes to Rgx/Mac, or uppercase, as I do now see that existing usages have mixed case, it would indeed be most consistent to match how it is written elsewhere.

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 14, 2022

sample results

01:12:12.754 CMD: rgxclients
01:12:12.766 MQT: stat/nat2/RESULT = {"RgxClients":{}}
01:12:32.271 CMD: rgxclients
01:12:32.286 MQT: stat/nat2/RESULT = {"RgxClients":{"ac:5f:3e:50:65:bb":{"IPAddress":"192.168.4.2"}}}
01:18:07.561 CMD: rgxclients
01:18:07.578 MQT: stat/nat2/RESULT = {"RgxClients":{"ac:5f:3e:50:65:bb":{"IPAddress":"192.168.4.2"},"8a:2b:91:73:f7:3d":{"IPAddress":"192.168.4.3"}}}

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 14, 2022

ci errors don't look like caused by my code.
Wanted to add RSSI info anyways (hope you don't mind), so...

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 14, 2022

result with RSSI looks like this:

{"RgxClients":{"4c:eb:d6:77:f7:60":{"IPAddress":"192.168.4.2","RSSI":-84},"ac:5f:3e:50:65:bb":{"IPAddress":"192.168.4.4","RSSI":-53}}}

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 14, 2022

hope it's good to go now...

@arendst
Copy link
Owner

arendst commented Nov 14, 2022

Looks very good.

A final suggestion, the zigbee crew are using their "mac" address without the colon like

{"BLEAlias":{"001A2208C9FA":"eq3_dining_room","001A2208B97D":"eq3_living_room","001A2214ADC8":"eq3_master_bedroom","001A2214A6DF":"eq3_kitchen","001A2214ADBF":"eq3_master_bathroom","001A2208CFD3":"eq3_walk_in","001A2214A73A":"eq3_guest_toilet","001A2214B86E":"eq3_office","A4C138F28078":"xiaomi_living_room","A4C138E28B41":"xiaomi_master_bathroom","A4C13876598C":"xiaomi_master_bedroom","A4C13880F246":"xiaomi_walk_in"}}

Although colon is a valid JSON name character I think this is a better approach with regards to Tasmota rule processing too.

If you agree, pls make this last change (in uppercase too) and I'll merge ;-)

@sillyfrog
Copy link

This is looking great, I'll continue to keep an eye on the progress, however it looks well in hand! :)

@joba-1
Copy link
Contributor Author

joba-1 commented Nov 14, 2022

13:01:13.167 MQT: stat/nat2/RESULT = {"RgxClients":{"AC5F3E5065BB":{"IPAddress":"192.168.4.2","RSSI":-53},"4CEBD677F760":{"IPAddress":"192.168.4.3","RSSI":-85}}}
I could read and distinguish macs much better with lower case, colon separated format, but hey, uniformity also has its value, maybe I get used to it...

@arendst arendst merged commit c63aad4 into arendst:development Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants