Align MTORR concentrator constants with Silicon Labs SDK defaults#718
Align MTORR concentrator constants with Silicon Labs SDK defaults#718TheJulianJES wants to merge 1 commit into
Conversation
The current values were inherited from very old EmberZNet sample code and don't match what the Silicon Labs SDK or other modern adapters use: | Param | Old | New | SDK default | SDK range | |-----------------------------|-------|-----|-------------|-----------| | MTOR_MIN_INTERVAL | 60 | 10 | 10 | 1..60 | | MTOR_MAX_INTERVAL | 3600 | 60 | 60 | 30..300 | | MTOR_ROUTE_ERROR_THRESHOLD | 8 | 3 | 3 | 1..100 | | MTOR_DELIVERY_FAIL_THRESHOLD| 8 | 1 | 1 | 1..100 | `MTOR_MAX_INTERVAL = 3600` was the worst of the four — 12× the SDK documented maximum. The NCP accepted it on the wire (uint16) but it meant up to an hour between MTORR broadcasts, leaving devices that lost their route to the concentrator stranded for that long. The higher error/failure thresholds also slowed reactive re-broadcasts on topology changes. The SDK plugin defaults are documented in `protocol/zigbee/app/framework/plugin/concentrator/config/concentrator-config.h`. zigbee-herdsman's ember adapter applies the same defaults and validates against the same ranges.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #718 +/- ##
=======================================
Coverage 99.54% 99.54%
=======================================
Files 61 61
Lines 4147 4147
=======================================
Hits 4128 4128
Misses 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zigpy-review-bot
left a comment
There was a problem hiding this comment.
Review: MTORR constant alignment with Silicon Labs SDK defaults
Verified each of the four new values against the canonical Silicon Labs concentrator plugin defaults. They match exactly:
| Constant | New value | SDK default | SDK range |
|---|---|---|---|
MTOR_MIN_INTERVAL |
10 | 10 | 5..60 |
MTOR_MAX_INTERVAL |
60 | 60 | 30..300 |
MTOR_ROUTE_ERROR_THRESHOLD |
3 | 3 | 1..100 |
MTOR_DELIVERY_FAIL_THRESHOLD |
1 | 1 | 1..100 |
Source: the EmberZNet concentrator plugin plugin.properties (minTimeBetweenBroadcastsSeconds.default=10, .type=NUMBER:5,60; maxTimeBetweenBroadcastsSeconds.default=60, .type=NUMBER:30,300; routeErrorThreshold.default=3, .type=NUMBER:1,100; deliveryFailureThreshold.default=1, .type=NUMBER:1,100).
zigbee-herdsman's ember adapter (src/adapter/ember/adapter/emberAdapter.ts, DEFAULT_STACK_CONFIG) applies the same defaults except CONCENTRATOR_MIN_TIME = 5 (vs. 10 here). Both 5 and 10 are inside the SDK's valid 5..60 range, and 10 is the documented SDK default, so matching the SDK over the herdsman tweak seems the right choice.
Notes
-
PR-body range nit. The body lists
MTOR_MIN_INTERVALrange as1..60. The SDK plugin properties file actually says5..60(.type=NUMBER:5,60). Doesn't affect the chosen value (10) but worth correcting in the PR description before merge. -
Behavioral impact of the
MAX_INTERVALchange is the largest of the four. Dropping from 3600s to 60s means MTORR broadcasts go from at most once per hour to up to once per minute — a 60x increase in the worst-case broadcast rate. Silicon Labs' own networking-concepts doc flags this exact tradeoff: "the smaller the MTORR interval and error thresholds, the faster and more responsive of many-to-one / source route establishment and repair, at the expense of increased network traffic. If the network is large and/or dense, frequent broadcasts and their echoes can quickly become overwhelming, so choose these parameters with care." That said, the new value is the documented SDK default and is what real EmberZNet deployments ship with, so the prior 3600 was the outlier — it sat 12x above the SDK's documented maximum (300) and would have left devices that lost their route to the coordinator stranded for up to an hour. The change is net-positive; just worth flagging in release notes so anyone running a very large mesh knows the broadcast cadence changed. -
Threshold drop from 8 to 1 for
DELIVERY_FAILis also aggressive. A single delivery failure now triggers an MTORR re-broadcast, vs. eight previously. This is again the SDK default and what herdsman uses, so it's correct, but combined with theMAX_INTERVALchange it shifts the network significantly toward "reactive" behavior. Mentioning this in release notes alongside #716/#717/#719 would help anyone bisecting later regressions. -
Tests. Constants-only change; existing
test_set_enable_source_routing/test_set_disable_source_routingintests/test_ezsp.pyonly assert on call structure, not numeric values, so no test churn is expected and none is present. Concentrator-related tests pass locally. -
Coordination with sibling PRs. This is one of four related source-routing tuning PRs (#716 RESCHEDULE discovery mode, #717 MTORR-countdown logging, #719 AODV on source-routed unicasts). The combined effect on broadcast/discovery cadence is meaningful — recommend landing them together or at least cross-linking in changelog entries so users can correlate behavior changes to the right release.
Verdict
The values are correct per the authoritative SiLabs source. The DRAFT flag's question ("is this really accurate?") can be answered: yes, all four match the documented SDK defaults exactly. Recommend fixing the 1..60 -> 5..60 range typo in the PR body, then this is ready to come out of draft.
References:
- SiLabs concentrator plugin defaults:
app/framework/plugin/concentrator/plugin.properties(EmberZNet) - SiLabs Zigbee Networking Concepts: https://docs.silabs.com/zigbee/8.2.2/zigbee-concepts/networking-concepts (MTORR tradeoff discussion)
- zigbee-herdsman ember adapter
DEFAULT_STACK_CONFIG: https://github.com/Koenkk/zigbee-herdsman/blob/master/src/adapter/ember/adapter/emberAdapter.ts
| MTOR_MAX_INTERVAL = 3600 | ||
| MTOR_ROUTE_ERROR_THRESHOLD = 8 | ||
| MTOR_DELIVERY_FAIL_THRESHOLD = 8 | ||
| MTOR_MIN_INTERVAL = 10 |
There was a problem hiding this comment.
Matches the SiLabs EmberZNet concentrator plugin default (minTimeBetweenBroadcastsSeconds.default=10, valid range 5..60). Note the PR body lists the range as 1..60 -- it's actually 5..60 per plugin.properties. Doesn't change the value, just the range column in the description.
| MTOR_ROUTE_ERROR_THRESHOLD = 8 | ||
| MTOR_DELIVERY_FAIL_THRESHOLD = 8 | ||
| MTOR_MIN_INTERVAL = 10 | ||
| MTOR_MAX_INTERVAL = 60 |
There was a problem hiding this comment.
This is the biggest behavioral shift in the PR: 60x more frequent worst-case MTORR broadcasts than before (3600s -> 60s). The new value matches the SDK default exactly (maxTimeBetweenBroadcastsSeconds.default=60, range 30..300) and the prior 3600 was 12x above the SDK's documented maximum, so this is a correction rather than a tuning change. Worth a release-note line since the on-air broadcast cadence will visibly change for users on large meshes -- SiLabs' networking-concepts doc explicitly warns that small intervals can be overwhelming on large/dense networks, even though their own default is 60s.
| MTOR_MIN_INTERVAL = 10 | ||
| MTOR_MAX_INTERVAL = 60 | ||
| MTOR_ROUTE_ERROR_THRESHOLD = 3 | ||
| MTOR_DELIVERY_FAIL_THRESHOLD = 1 |
There was a problem hiding this comment.
Aggressive drop from 8 to 1 -- a single delivery failure now triggers MTORR re-broadcast. This is the SiLabs default (deliveryFailureThreshold.default=1) and what zigbee-herdsman uses, so the value is correct. Combined with the MAX_INTERVAL drop, the network becomes substantially more reactive to topology changes; worth calling out alongside #716/#717/#719 in release notes.
DRAFT: Check if this is really accurate.
AI summary (check if accurate)
The current values were inherited from very old EmberZNet sample code and don't match what the Silicon Labs SDK or other modern adapters use:
MTOR_MAX_INTERVAL = 3600was the worst of the four — 12× the SDK documented maximum. The NCP accepted it on the wire (uint16) but it meant up to an hour between MTORR broadcasts, leaving devices that lost their route to the concentrator stranded for that long. The higher error/failure thresholds also slowed reactive re-broadcasts on topology changes.The SDK plugin defaults are documented in
protocol/zigbee/app/framework/plugin/concentrator/config/concentrator-config.h. zigbee-herdsman's ember adapter applies the same defaults and validates against the same ranges.