refactoring mdns request parsing #340
Conversation
mdns responses now doenst rely on the url starting with "elrs" and its not supporting multiple elrs answers in a response
mdns responses now doenst rely on the url starting with "elrs" and its now supporting multiple elrs answers in a response
…into feature/refactor_mdns_response
There was a problem hiding this comment.
Changing the way we identify elrs devices from checking if the name starts with elrs to instead check if vender = elrs is good, and is the more proper way to identify the elrs device.
The rest of the refactoring seems to have been done under the impression that there could be multiple elrs devices included in a single ResponsePacket, which isn't going to occur.
| @@ -120,88 +121,104 @@ export default class MulticastDnsService { | |||
|
|
|||
| // check if any of the answers on the response have a name that starts with | |||
| // 'elrs_', which indicates that it is an elers device | |||
There was a problem hiding this comment.
This comment can be removed since it is no longer applicable
| const dns: string = srvResponse.data?.target; | ||
| const port: number = srvResponse.data?.port; | ||
|
|
||
| if (srvResponse) { |
There was a problem hiding this comment.
No need to check "if (srvResponse)" since it was already checked a few lines above.
There was a problem hiding this comment.
missed that, removed
| (answer) => answer.type === 'SRV' | ||
| ) as SrvAnswer[]; | ||
|
|
||
| const srvResponse: SrvAnswer | undefined = srvResponses.find((i) => { |
There was a problem hiding this comment.
There is no need to check for the name to match, a responsePacket is only going to contain information about a single response, there will never be multiple devices included in a single mdns response. It doesn't hurt anything that it is being checked for, just isn't necessary.
There was a problem hiding this comment.
i thought there would be a possibility of multiple responses in a answer package. I changed them to "find" instead of "filter"
| if (srvResponse) { | ||
| const aResponse: StringAnswer | undefined = items.find( | ||
| (answer) => | ||
| answer.type === 'A' && answer.name === srvResponse.data.target |
There was a problem hiding this comment.
Same as above for the srv response, really isn't any need to check for the name to match.
|
I changed all your suggestions. Wasnt aware, that a single response can only contain one answer. |
|
Yes, multicast-dns emits a response event for each individual mdns response it receives, that response can contain multiple record types like TXT, A, SRC, etc. but they all pertain to a single response from a single service. That is why the original code didn't match on name, it just looked for the TXT, A, and SRC types in the ResponsePacket since all those records correspond to a response from a single elrs device. |
Understood, so the latest changes are reverting that, that's fine now? |
mdns responses now doenst rely on the url starting with "elrs" and its now supporting multiple elrs answers in a response