Decouple transport from session management. #1424
Conversation
As we make transports generic, the source should be a peer address rather than a packet info. This is because different transports may have different address information (specifically type may be different).
The intent is to merge session management as more transports are being added. Specifically having a single session management for both IPv4 and IPv6 for ESP.
andy31415
requested review from
anush-apple,
BroderickCarlin,
chrisdecenzo,
hawk248,
jelderton,
robszewczyk,
saurabhst and
woody-apple
as code owners
|
|
This comment has been minimized.
This comment has been minimized.
|
LGTM |
src/transport/Tuple.h
Outdated
| class Tuple : public Base | ||
| { | ||
| public: | ||
| CHIP_ERROR SendMessage(const MessageHeader & header, const Transport::PeerAddress & address, |
There was a problem hiding this comment.
Please include method Doxygen documentation.
There was a problem hiding this comment.
This is an override of the Base, which includes DoxyGen.
There was a problem hiding this comment.
Can we use the Doxygen @overload to indicate that?
There was a problem hiding this comment.
Done. Was not aware @overload is a thing.
That being said, this is redundant: method already has an 'override' C++ tag for this purpose. Then again unsure if Doxygen notices.
src/transport/Tuple.h
Outdated
| return SendMessageImpl<0>(header, address, msgBuf); | ||
| } | ||
|
|
||
| bool CanSendToPeer(const Transport::PeerAddress & address) override { return CanSendToPeerImpl<0>(address); } |
There was a problem hiding this comment.
Please include method Doxygen documentation.
Style nit: please don't one-line the declaration and implementation.
There was a problem hiding this comment.
Restyler fights me here.
There was a problem hiding this comment.
@woody-apple or @rwalker-apple, thoughts or insights here? Do we have a clang-format option that enforces one-liners? If so, can we disable it?
There was a problem hiding this comment.
it may be a side-effect of line-wrapping, will look
There was a problem hiding this comment.
isn't achievable with clang-format unless we want to change line width (I don't)
There was a problem hiding this comment.
Yeah, no, I don't think we should change line width either. However, I find it surprising that if @andy31415 makes a method function body in a header in a "traditional" non-one-liner style:
rvalue name(params, ...)
{
return someother_function(params);
}
that clang-format / restyler forces this to a one-liner.
src/transport/Tuple.h
Outdated
| * - BLE transports only support BLE. TCP/UDP support IP addresses | ||
| * - Multicasts only supported by UDP | ||
| * | ||
| * @param TransporTypes the ORDERED list of transport types grouped together. Order matters in |
There was a problem hiding this comment.
I think @param needs to be @tparam, per here.
Also TransporTypes -> TransportTypes.
There was a problem hiding this comment.
There are other template parameter instances below that I believe need to be likewise tweaked.
There was a problem hiding this comment.
Updated. VSCode colors @param but not @tparam so I was unsure if the tparam is a thing or not. A search seems to indicate it exists, so I will trust the search. It also turns out (as per below) that I cannot actually verify since our doc build is toast on master
src/transport/Tuple.h
Outdated
| class Tuple : public Base | ||
| { | ||
| public: | ||
| /** @overload */ |
There was a problem hiding this comment.
Thanks. When running make doc, does the right thing happen or does Doxygen squawk?
There was a problem hiding this comment.
make doc yells at me as 'does not exist' and 'docdist' also complains. There is a makefile under a 'docs' directory however make in there does not do anything.
I am not sure how to build docs :( Still looking though.
There was a problem hiding this comment.
created #1486 for this: just tried on master checkout and help claims make doc builds docs, however instead I get an error.
There was a problem hiding this comment.
Got the docs to build and as a result I made some code formatting changes.
Also found that @overload does more harm than good: if I add it, doxygen adds automatically a 'This is an overloaded member function, provided for convenience. It differs from the above function only in what argument(s) it accepts.' where as if I do not add it, it copies over the doxygen from the base class (which seems to be what we want.
I dropped overloads as a result.
There was a problem hiding this comment.
Great, thanks for checking it out.
|
@andy31415 can we rebase, and fix conflicts here? |
Problem
Currently a separate session manager is created for every transport (e.g. ESP creates 2: one for ipv4 and one for ipv6). There should be a central session and encryption management rather than per protocol.
Summary of Changes
Decouples transport from session management.
fixes #1339