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

Put R_GetProductId() and isTuyaManufacturerName() in own module #4717

Merged
merged 5 commits into from
Apr 8, 2021

Conversation

manup
Copy link
Member

@manup manup commented Apr 6, 2021

New files product_match.[cpp,h] to provide functions to identify products.

The PR removes resource.cpp dependency on the whole plugin which was included via tuya.h.

@Smanar I haven't looked through all PRs in detail, but if some of them extend the product map this would conflict.
I can resolve the conflicts when mergin the PRs.

@manup manup requested a review from Smanar April 6, 2021 15:43
@ebaauw ebaauw self-requested a review April 6, 2021 16:11
Copy link
Collaborator

@ebaauw ebaauw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same logic as I implemented for the LIDL devices in #3975 back in December:

struct lidlDevice {
const char *zigbeeManufacturerName;
const char *zigbeeModelIdentifier;
const char *manufacturername;
const char *modelid;
};
static const lidlDevice lidlDevices[] = { // Sorted by zigbeeManufacturerName
{ "_TYZB01_bngwdjsr", "TS1001", "LIDL Livarno Lux", "HG06323" }, // Remote Control
{ "_TZ3000_1obwwnmq", "TS011F", "LIDL Silvercrest", "HG06338" }, // Smart USB Extension Lead (EU)
{ "_TZ3000_49qchf10", "TS0502A", "LIDL Livarno Lux", "HG06492C" }, // CT Light (E27)
{ "_TZ3000_9cpuaca6", "TS0505A", "LIDL Livarno Lux", "14148906L" }, // Stimmungsleuchte
{ "_TZ3000_dbou1ap4", "TS0505A", "LIDL Livarno Lux", "HG06106C" }, // RGB Light (E27)
{ "_TZ3000_el5kt5im", "TS0502A", "LIDL Livarno Lux", "HG06492A" }, // CT Light (GU10)
{ "_TZ3000_gek6snaj", "TS0505A", "LIDL Livarno Lux", "14149506L" }, // Lichtleiste
{ "_TZ3000_kdi2o9m6", "TS011F", "LIDL Silvercrest", "HG06337" }, // Smart plug (EU)
{ "_TZ3000_kdpxju99", "TS0505A", "LIDL Livarno Lux", "HG06106A" }, // RGB Light (GU10)
{ "_TZ3000_oborybow", "TS0502A", "LIDL Livarno Lux", "HG06492B" }, // CT Light (E14)
{ "_TZ3000_odygigth", "TS0505A", "LIDL Livarno Lux", "HG06106B" }, // RGB Light (E14)
{ "_TZ3000_riwp3k79", "TS0505A", "LIDL Livarno Lux", "HG06104A" }, // LED Light Strip
{ "_TZE200_s8gkrkxk", "TS0601", "LIDL Livarno Lux", "HG06467" }, // Smart LED String Lights (EU)
{ nullptr, nullptr, nullptr, nullptr }
};
static const lidlDevice *getLidlDevice(const QString &zigbeeManufacturerName)
{
const lidlDevice *device = lidlDevices;
while (device->zigbeeManufacturerName != nullptr)
{
if (zigbeeManufacturerName == QLatin1String(device->zigbeeManufacturerName))
{
return device;
}
device++;
}
return nullptr;
}
static bool isLidlDevice(const QString &zigbeeModelIdentifier, const QString &manufacturername)
{
const lidlDevice *device = lidlDevices;
while (device->zigbeeManufacturerName != nullptr)
{
if (zigbeeModelIdentifier == QLatin1String(device->zigbeeModelIdentifier) &&
manufacturername == QLatin1String(device->manufacturername))
{
return true;
}
device++;
}
return false;
}

I call these methods when handling the Read Attributes Response/Report Attributes, to patch the manufacturername and modelid values to the logical names instead of the Tuya names, so the rest of the code can use these.

I think we should at least try and harmonise this and merge the implementations.

@SwoopX
Copy link
Collaborator

SwoopX commented Apr 6, 2021

Guys, please keep #4710 in mind here.

@Smanar
Copy link
Collaborator

Smanar commented Apr 6, 2021

I haven't looked through all PRs in detail, but if some of them extend the product map this would conflict.
I can resolve the conflicts when mergin the PRs.

Ok, else I can do it (there will be probably other conflict) but let me somes days betwen this PR validation and other.

@manup
Copy link
Member Author

manup commented Apr 6, 2021

I think we should at least try and harmonise this and merge the implementations.

Good point, as a start I'd like to move the lidl functions unchanged into product_map.cpp to get them out of the de_web_plugin.cpp.

The QString R_GetProductId(Resource*) doesn't change the manufacturer name / modelid, it only returns the mapped value and attaches the non public attr/productid item which could also be interpreted as prettier attr/modelid. The same can be done for the manufacturer name. Not changing the original value is handy at some places like isTuyaManufacturerName().

I think the map should be extended to also specify which values should be returned by the API so we have precise control and don't mess up with breaking changes. The values shown in REST GET responses would then just be retrieved via helper functions, for example:

QString GetApiManufacturerName(const Resource*);
QString GetApiModelId(const Resource*);

For most Tuya devices the cryptic values will be returned to not break clients. Perhaps we should expose the mapped values separately to API like attr/prettymanufacturer and attr/productid (or whatever they should be called) so everything is available but nothing breaks.

Sometimes like during pairing we don't have a Resource* (Sensor or LightNode) available, here we could use further functions:

QString GetPrettyManufacturerName(const QString &manufacturerName, const QString &modelId);
QString GetPrettyModelId(const QString &manufacturerName, const QString &modelId);

Note: If the map doesn't have an entry the input parameter is returned.

@manup
Copy link
Member Author

manup commented Apr 6, 2021

Ok, else I can do it (there will be probably other conflict) but let me somes days betwen this PR validation and other.

@Smanar I have added the entries from the PRs in commit d19005f

So in theory in your related PRs you just need to leave them out, hope this helps a bit.

No code change, just relocation.
@manup
Copy link
Member Author

manup commented Apr 6, 2021

I moved the LIDL device code from de_web_plugin.cpp to product_match.cpp. I think this can be merged for now and refactoring the two maps into into one interface can be done in a separate PR with some tests to verify nothing breaks.

@manup manup merged commit 8f2edad into dresden-elektronik:master Apr 8, 2021
@manup manup added this to the v2.11.0-beta milestone Apr 8, 2021
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.

4 participants