Skip to content

Conversation

@seriousme
Copy link
Contributor

This PR allows bride protocol versions (MQTT protocol+128) to be generated as discussed in moscajs/aedes#1042 (review)

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM

@robertsLando robertsLando requested a review from mcollina July 31, 2025 06:12
@robertsLando
Copy link
Member

Cool enough seems this spec is not present anywhere on MQTT specs, but can be found in other MQTT modules like: https://github.com/eclipse-mosquitto/mosquitto/blob/3cbe805e71ac41a2a20cc9b2ea6b3b619f49554a/lib/send_connect.c#L153

@seriousme
Copy link
Contributor Author

Indeed, it seems to be convention and not part of the formal spec.

@seriousme
Copy link
Contributor Author

The parser already supports it:

mqtt-packet/parser.js

Lines 171 to 173 in e39fb28

if (packet.protocolVersion >= 128) {
packet.bridgeMode = true
packet.protocolVersion = packet.protocolVersion - 128

Looking at the parser we might want to make it symmetrical and use:

protocolVersion: 4
bridgeMode: true

during generation as well.
Which seems a better solution than my current fix.

@robertsLando
Copy link
Member

@seriousme yep agree, go for that

@seriousme
Copy link
Contributor Author

I will push an update later today.

@seriousme seriousme marked this pull request as draft July 31, 2025 07:11
@seriousme
Copy link
Contributor Author

I did some more research and the feature was already present:

if (settings.bridgeMode) {
protocolVersion += 128
}

So we can close this PR.

@seriousme seriousme closed this Jul 31, 2025
@seriousme seriousme deleted the add-bridge-protovol-versions-in-generate branch July 31, 2025 13:42
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.

2 participants