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

chore: remove manuSpecificNodOnPilotWire that was moved to zigbee-herdsman-converters #1274

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

vmeurisse
Copy link
Contributor

Previous logic stopped on the first cluster with no manufacturerCode.

Fixes Koenkk/zigbee2mqtt#25333

@Nerivec
Copy link
Collaborator

Nerivec commented Dec 27, 2024

This looks like a very specific case of custom cluster ID clashing?
Can't we shuffle the Clusters object instead of de-optimizing the lookup (re-order manuSpecificNodOnPilotWire before manuSpecificAssaDoorLock)?
This new code will force lookup through the entire Clusters object every time, considering the amount of times this is called, de-optimizing it will have a rather large negative impact.

@Nerivec
Copy link
Collaborator

Nerivec commented Dec 27, 2024

Actually... @Koenkk can't we move these two (and others that may conflict) to custom clusters directly and remove the problem entirely?

@vmeurisse
Copy link
Contributor Author

vmeurisse commented Dec 28, 2024

On a Raspberry PI 4, the function takes 0.003ms for an empty list and 0.015ms to loop over the 132 clusters, measured on the average of 20 executions. I removed the break to make sure to loop over the complete list.

If perf is an issue, it might be better to preprocess the list on start to group if by id, as it is less error prone than relying on the cluster order in the source code.

Code used
function findClusterNameByID(id, manufacturerCode, clusters) {                                                               
    console.log(Object.keys(clusters).length);                                                                               
    let time = process.hrtime();                                                                                             
    let matchingManufacturer;                                                                                                
    let noManufacturer;                                                        
    let anyManufacturer;                                      
    for (const clusterName in clusters) {                     
        const cluster = clusters[clusterName];                
        if (cluster.ID === id) {                              
            // When a manufacturerCode is requested, match the first cluster with matching manufacturerCode,
            if (manufacturerCode && cluster.manufacturerCode === manufacturerCode) {                        
                matchingManufacturer = clusterName;                                                         
                // Otherwise, match the first cluster with no manufacturerCode,                             
            }                                                                                               
            else if (!noManufacturer && !cluster.manufacturerCode) {                                        
                noManufacturer = clusterName;                                                               
                // Finally, match the first cluster with a matching ID.                                     
            }                                                                                               
            else if (!anyManufacturer) {                                                                    
                anyManufacturer = clusterName;                                      
            }                                                                  
        }                                                                      
    }                                                                  
    const exactMatch = (manufacturerCode && matchingManufacturer) || (!manufacturerCode && noManufacturer);
    const clusterName = matchingManufacturer || noManufacturer || anyManufacturer;                         
    time = process.hrtime(time);                                                                           
    console.log('benchmark took %d seconds and %d nanoseconds', time[0], time[1]);                         
    return [clusterName, !exactMatch];                                                                     
}

@Koenkk
Copy link
Owner

Koenkk commented Dec 28, 2024

Yes it would be better to completely remove the clusters here (at least one of them) and move them into zhc. Example on how to do that: https://github.com/Koenkk/zigbee-herdsman-converters/blob/20c0958f6e99343040b334898de494fb97f18aa8/src/devices/sonoff.ts#L1105

@vmeurisse vmeurisse changed the title fix: correctly match manufacturerCode in findClusterNameByID chore: remove manuSpecificNodOnPilotWire that was moved to zigbee-herdsman-converters Dec 29, 2024
@vmeurisse
Copy link
Contributor Author

I moved the cluster definition in Koenkk/zigbee-herdsman-converters#8535

@Koenkk Koenkk merged commit 1c8d886 into Koenkk:master Dec 30, 2024
1 check passed
@Koenkk
Copy link
Owner

Koenkk commented Dec 30, 2024

Thanks!

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.

Cannot read status on SIN-4-FP-21_EQU
3 participants