Make CHIP devices advertise as operational devices via mDNS #4132
Conversation
… to make sure multiple start calls work
…g, so mark esp verbosity to verbose
|
Had a few bugfixes: invalid && return for builders, created separatte packetbuffer copy for every broadcast packet. |
pullapprove
bot
requested review from
BroderickCarlin,
bzbarsky-apple,
chrisdecenzo,
hawk248,
jelderton,
mspang,
rwalker-apple and
saurabhst
|
Now that I started using minMDNS beyond hardcoded examples, I expect I will want a few passes to make it a bit nicer (constructor addresses and settings seem hard to follow). For now I take this as 'this works' and then will iterate to add 'commisioning advertising' and make the thing a bit more readable. In parallel we can build the platform-dependent mDNS version (avahi and esp32 if applicable). minMDNS tries hard to only use static memory and never dynamic, which has sideffects in how pretty the code is. |
| { | ||
| char buff[32]; | ||
| snprintf(buff, sizeof(buff), "%d", value); | ||
| buff[sizeof(buff) - 1] = 0; |
There was a problem hiding this comment.
this one isn't necessary
also the largest int value should consume less than 32 bytes :-)
There was a problem hiding this comment.
Right ... even 64-bit is 20 chars at most. Wanted to not think about it since I imagine this code will be copied and pasted as if we ever want to format additional value types in here.
Do you think it would be useful to drop the line or are ok to keep as is?
| @@ -536,5 +536,31 @@ PacketBufferHandle PacketBufferHandle::PopHead() | |||
| return PacketBufferHandle(head); | |||
| } | |||
|
|
|||
| PacketBufferHandle PacketBufferHandle::CloneData() | |||
| { | |||
| if (!mBuffer->Next().IsNull()) | |||
There was a problem hiding this comment.
Could this be an assertion?
There was a problem hiding this comment.
I am unsure how assertions work. nlASSERT?
There was a problem hiding this comment.
All asserts in this file seem to be static. nlASSERT seem to be a noop in production (really? that seems odd, but this is what following #define seems to give me).
I would rather keep it as is as empty return.
|
Size increase report for "nrfconnect-example-build" from 0998211
Full report output |
|
Size increase report for "esp32-example-build" from 0998211
Full report output |
|
@andy31415 Is the size of the codesize increase here about as expected? |
I don't have much control on the sizes. Flash/txt seems ok, BSS size seems a bit large. I expect as we iterate we may find places where we can decrease things. |
* SystemPacketBuffer.h cleanup #### Problem Code should use `PacketBufferHandle` rather than `PacketBuffer *`. Some existing methods should be removed or made private so that code does not have unnecessary access to the raw pointer. #### Summary of Changes - convert public use of `PacketBufferHandle::Adopt()`. - convert internal use of `Adopt()` to private `Free()`. - renamed `PacketBuffer::Create()` factory to `Adopt()`. - rename private `PacketBuffer::Next()` to `ChainedBuffer()`. - explicity note `PacketBuffer::Next_ForNow()` remaining use in TLV. - make `PacketBuffer::Free()` private. - make `PacketBuffer::Consume()` private. - make `PacketBuffer::AddToEnd_ForNow()` private (still used in tests). - remove `PacketBuffer::FreeHead_ForNow()`. - remove `operator*`. Part of issue #2707 - Figure out a way to express PacketBuffer ownership in the type system * Fix build (crossed paths with #4132) * Remove PacketBufferHandle::Free() * Fixes for #4241
Problem
Need CHIP devices to respond to mDNS properly.
Summary of Changes
Replaces DiscoveryManager (which has avahi/esp32 implementation only) with an abstracted-away minMDNS implementation for advertising. Tested via: