Implement and publish mDNS on esp32 #3452

gjc13 · 2020-10-27T06:24:42Z

Problem

mDNS is not implemented on esp32 platform.

Summary of Changes

Implement the platform mDNS api on esp32.
Add publisher api.
Publish device information via mDNS.

examples/temperature-measurement-app/esp32/main/ResponseServer.cpp

examples/wifi-echo/server/esp32/main/DeviceCallbacks.cpp

examples/wifi-echo/server/esp32/main/EchoServer.cpp

src/lib/protocols/mdns/Publisher.cpp

src/lib/protocols/mdns/Publisher.h

src/platform/ESP32/MdnsImpl.cpp

rwalker-apple · 2020-10-27T14:59:22Z

src/platform/ESP32/MdnsImpl.cpp

+exit:
+    if (items != nullptr)
+    {
+        chip::Platform::MemoryFree(items);


this could be allocated on the stack, I think

items is an array with variable length so we need to allocate from heap.

C++ stack allocation based on a parameter works fine (just don't use sizeof() on the resulting space :-( )

alloca.cpp:

#include <stdio.h> #include <stdlib.h> #include <string.h> int main(int argc, const char * const * argv) { int len = atoi(argv[1]); char before[1]; char buf[len]; char after[1]; printf("before: %p\n" "buf: %p\n" "after: %p\n" "sizeof(buf): %zu\n", before, buf, after, sizeof(buf)); return 0; }

$ g++ --std=c++11 alloca.cpp -o alloca && ./alloca 0x100 before: 0x7ffeee3169f3 buf: 0x7ffeee3169b0 after: 0x7ffeee3169df sizeof(buf): 0

This is a non-standard extension and fails if you build with -Wvla, though.

rwalker-apple · 2020-10-27T15:01:50Z

src/platform/ESP32/MdnsImpl.cpp

+    return error;
+}
+
+CHIP_ERROR ChipMdnsStopPublish()


I just noticed there's no "remove service" API at the CHIP level. how will we handle temporary registrations?

Ack, will add in other PR.

rwalker-apple · 2020-10-27T15:10:28Z

src/lib/protocols/mdns/Publisher.cpp

+
+CHIP_ERROR Publisher::Init()
+{
+    return ChipMdnsInit(HandleMdnsInit, HandleMdnsError, this);


this implies that Publisher can be the only client of ChipMdns?

Yes. We can add new methods to this class for future feature requests.

which class?

The Publisher class. I plan to handle all the service registration things here.

tcarmelveilleux

The spec for discovery is already merged. The format should match the available spec text.

tcarmelveilleux · 2020-10-28T03:01:00Z

src/lib/protocols/mdns/Publisher.cpp

+    VerifyOrExit(mInitialized, error = CHIP_ERROR_INCORRECT_STATE);
+    ChipLogProgress(Discovery, "setup mdns service");
+    SuccessOrExit(error = chip::DeviceLayer::ConfigurationMgr().GetSetupDiscriminator(discriminator));
+    sprintf(service.mName, "CHIP-%04d", discriminator);


This type of non-length-limited (e.g, sprintf/strcpy instead of snprintf/strncpy) is strongly discouraged. Pigweed's pw_string module has super-lightweight safe wrappers to do string formatting and concatenation over char arrays that is size-safe.

CHIP-xxxx is not the correct format. The format is settled, see https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/247

The publish format has been modified according to the spec. However, subtypes are not supported well in embedded systems (esp and lwip). We currently only publish the xxxxx._chipc._udp entry.

The fact that lwip and ESP default mDNS libraries are insufficient is separate from whether the codebase should support what the spec defines. I think it would be straightforward to use a separate good-features mDNS library on embedded to avoid hitting different limitations of underlying platforms that today provide extremely limited mDNS/DNS-SD support.

src/lib/protocols/mdns/Publisher.cpp

src/platform/ESP32/MdnsImpl.cpp

tcarmelveilleux · 2020-10-28T03:06:45Z

src/platform/ESP32/MdnsImpl.cpp

+    return mdns_service_remove_all() == ESP_OK ? CHIP_NO_ERROR : CHIP_ERROR_INTERNAL;
+}
+
+CHIP_ERROR ChipMdnsBrowse(const char * type, MdnsServiceProtocol protocol, chip::Inet::InterfaceId interface,


Strongly recommend against browsing on embedded device. If it is done, it should be done with unicast-only bit set in request. Is this possible to do with the mDNS library used?

src/platform/ESP32/MdnsImpl.cpp

tcarmelveilleux · 2020-10-28T15:09:40Z

src/lib/protocols/mdns/Publisher.cpp

+    char hostname[13]; // Hostname will be the hex representation of mac.
+    CHIP_ERROR error;
+
+    SuccessOrExit(error = chip::DeviceLayer::ConfigurationMgr().GetPrimaryWiFiMACAddress(mac));


This only works for Wifi, not thread. Thread EIDs are 64 bits. There should be no Wifi config directly read from common mDNS publisher, since for Ethernet based devices, there won't be a Wifi address. Please have a configuration method for MAC address (which may rotate over time, as well).

The current platform api only provides Thread EID and wifi mac address. I prefer to update the paltform api to provide meta information about the underlying connectivity (Thread/wifi/Ethernet enabled or not).

src/lib/protocols/mdns/Publisher.cpp

tcarmelveilleux · 2020-10-28T15:18:25Z

src/lib/protocols/mdns/Publisher.cpp

+    MdnsService service;
+    CHIP_ERROR error = CHIP_NO_ERROR;
+
+    SuccessOrExit(error = chip::DeviceLayer::ConfigurationMgr().GetFabricId(fabricId));


This is again something that should likely not use singleton values, but have them passed down, in case the multiadmin conversations boil down to having > 1 device ID per physical node (which is not impossible).

Also prefer to modify the platform api to iterate through the fabric/device ids.

tcarmelveilleux

Approved based on offline conversations that this PR is OK for demos, but there will need to be improvements to make the DNS-SD subsystem more scalable and have less assumptions.

Please fix the VID/PID format.

src/lib/protocols/mdns/Publisher.h

src/platform/ESP32/MdnsImpl.cpp

github-actions · 2020-10-29T04:43:58Z

Size increase report for "esp32-example-build" from 6a5b193

File	Section	File	VM
chip-wifi-echo.elf	.flash.text	22144	22144
chip-wifi-echo.elf	.dram0.bss	0	1904
chip-wifi-echo.elf	.flash.rodata	676	676
chip-wifi-echo.elf	[2 Others]	48	8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

sections,vmsize,filesize
.debug_info,0,250452
.debug_line,0,89699
.debug_loc,0,39035
.flash.text,22144,22144
.debug_abbrev,0,13813
.debug_str,0,10389
.debug_ranges,0,3704
.strtab,0,3655
[Unmapped],0,3412
.debug_frame,0,3260
.symtab,0,2160
.dram0.bss,1904,0
.debug_aranges,0,1216
.shstrtab,0,757
.flash.rodata,676,676
.xt.prop._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE24_GetManufacturerDeviceIdERy,0,100
.xt.prop._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetFabricIdERy,0,88
.xt.prop._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetDeviceIdERy,0,76
.xt.lit._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE12_GetFabricIdERy,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal31GenericConfigurationManagerImplINS0_24ConfigurationManagerImplEE24_GetManufacturerDeviceIdERy,0,48
[2 Others],8,48

github-actions · 2020-10-29T04:44:27Z

Size increase report for "nrfconnect-example-build" from 6a5b193

File	Section	File	VM
chip-shell.elf	rodata	8	4
chip-lock.elf	rodata	8	4
chip-lighting.elf	rodata	8	4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,111
.debug_str,0,21
rodata,4,8
.debug_line,0,1
.debug_loc,0,-5

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,183
.debug_str,0,21
rodata,4,8
.debug_line,0,1
.debug_loc,0,-5

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,183
.debug_str,0,21
rodata,4,8
.debug_line,0,1
.debug_loc,0,-5

github-actions · 2020-10-29T04:44:37Z

Size increase report for "gn_qpg6100-example-build" from 6a5b193

File	Section	File	VM
chip-qpg6100-lock-example.out	.rodata	4	4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lock-example.out and ./pull_artifact/chip-qpg6100-lock-example.out:

sections,vmsize,filesize
.debug_info,0,87
.debug_str,0,18
.rodata,4,4
.debug_loc,0,-1
[Unmapped],0,-4

yufengwangca · 2020-10-29T17:23:04Z

please move src/lib/protocols/mdns/ to src/lib/mdns.

The directory src/lib/protocols/ is hosting Weave profiles, not network protocols. Those two directories src/lib/protocols/ and src/lib/message contain the wml based messaging stack, we plan to clean them up after we cherry pick all features to current messaging stack.

boring-cyborg bot added esp32 examples lib linux platform labels Oct 27, 2020

gjc13 force-pushed the esp-mdns branch 3 times, most recently from 8849a42 to d28f044 Compare October 27, 2020 07:09

pullapprove bot requested review from andy31415, BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple and saurabhst October 27, 2020 07:09

esp32 mdns impl

5d8ad4c

gjc13 force-pushed the esp-mdns branch from d28f044 to 5d8ad4c Compare October 27, 2020 07:14

andy31415 approved these changes Oct 27, 2020

View reviewed changes

rwalker-apple reviewed Oct 27, 2020

View reviewed changes

src/platform/ESP32/MdnsImpl.cpp Show resolved Hide resolved

BroderickCarlin approved these changes Oct 27, 2020

View reviewed changes

rwalker-apple reviewed Oct 27, 2020

View reviewed changes

src/platform/ESP32/MdnsImpl.cpp Show resolved Hide resolved

rwalker-apple reviewed Oct 27, 2020

View reviewed changes

src/platform/ESP32/MdnsImpl.cpp Outdated Show resolved Hide resolved

rwalker-apple reviewed Oct 27, 2020

View reviewed changes

rwalker-apple mentioned this pull request Oct 27, 2020

No RemoveService() API in CHIP's Mdns API #3469

Closed

rwalker-apple reviewed Oct 27, 2020

View reviewed changes

tcarmelveilleux requested changes Oct 28, 2020

View reviewed changes

rwalker-apple mentioned this pull request Oct 28, 2020

Mdns has no "remove service" facility #3484

Closed

fix review comments

454c300

gjc13 force-pushed the esp-mdns branch 3 times, most recently from c992b1b to 536d81b Compare October 28, 2020 10:52

tcarmelveilleux reviewed Oct 28, 2020

View reviewed changes

tcarmelveilleux approved these changes Oct 28, 2020

View reviewed changes

mspang approved these changes Oct 29, 2020

View reviewed changes

src/lib/protocols/mdns/Publisher.h Outdated Show resolved Hide resolved

src/platform/ESP32/MdnsImpl.cpp Outdated Show resolved Hide resolved

gjc13 force-pushed the esp-mdns branch from 536d81b to c0f8719 Compare October 29, 2020 04:17

use format in spec

388c5e7

gjc13 force-pushed the esp-mdns branch from c0f8719 to 388c5e7 Compare October 29, 2020 04:23

jelderton approved these changes Oct 29, 2020

View reviewed changes

mspang merged commit 5ad6518 into project-chip:master Oct 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement and publish mDNS on esp32 #3452

Implement and publish mDNS on esp32 #3452

gjc13 commented Oct 27, 2020

rwalker-apple Oct 27, 2020

gjc13 Oct 28, 2020

rwalker-apple Oct 28, 2020 •

edited

Loading

mspang Oct 28, 2020

rwalker-apple Oct 27, 2020

gjc13 Oct 28, 2020

rwalker-apple Oct 27, 2020

gjc13 Oct 28, 2020

rwalker-apple Oct 28, 2020

gjc13 Oct 29, 2020

tcarmelveilleux left a comment

tcarmelveilleux Oct 28, 2020

tcarmelveilleux Oct 28, 2020

gjc13 Oct 28, 2020

tcarmelveilleux Oct 28, 2020

tcarmelveilleux Oct 28, 2020

tcarmelveilleux Oct 28, 2020

gjc13 Oct 29, 2020

tcarmelveilleux Oct 28, 2020

gjc13 Oct 29, 2020

tcarmelveilleux left a comment

github-actions bot commented Oct 29, 2020

github-actions bot commented Oct 29, 2020

github-actions bot commented Oct 29, 2020

yufengwangca commented Oct 29, 2020

Implement and publish mDNS on esp32 #3452

Implement and publish mDNS on esp32 #3452

Conversation

gjc13 commented Oct 27, 2020

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwalker-apple Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcarmelveilleux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcarmelveilleux left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2020

github-actions bot commented Oct 29, 2020

github-actions bot commented Oct 29, 2020

yufengwangca commented Oct 29, 2020

rwalker-apple Oct 28, 2020 •

edited

Loading