Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Final PacketBufferHandle cleanup. #4364

Merged
merged 6 commits into from
Jan 26, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion examples/chip-tool/commands/common/Command.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ class Command
using ChipDevice = ::chip::Controller::Device;
using PeerAddress = ::chip::Transport::PeerAddress;
using IPAddress = ::chip::Inet::IPAddress;
using PacketBuffer = ::chip::System::PacketBuffer;
using PacketBufferHandle = ::chip::System::PacketBufferHandle;
using NodeId = ::chip::NodeId;

Expand Down
2 changes: 1 addition & 1 deletion src/app/Command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ chip::TLV::TLVWriter & Command::CreateCommandDataElementTLVWriter()
mCommandDataBuf = chip::System::PacketBuffer::New();
if (mCommandDataBuf.IsNull())
{
ChipLogDetail(DataManagement, "Unable to allocate PacketBuffer");
ChipLogDetail(DataManagement, "Unable to allocate packet buffer");
}

mCommandDataWriter.Init(mCommandDataBuf.Retain());
Expand Down
2 changes: 1 addition & 1 deletion src/app/encoder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * a
PacketBufferHandle payload = PacketBuffer::NewWithAvailableSize(kMaxBufferSize); \
if (payload.IsNull()) \
{ \
ChipLogError(Zcl, "Could not allocate PacketBuffer while trying to encode %s command", kName); \
ChipLogError(Zcl, "Could not allocate packet buffer while trying to encode %s command", kName); \
return payload; \
} \
\
Expand Down
2 changes: 1 addition & 1 deletion src/app/util/chip-message-send.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
#include "chip-message-send.h"

#include <assert.h>
#include <inet/InetLayer.h> // PacketBuffer and the like
#include <inet/InetLayer.h>
#include <support/logging/CHIPLogging.h>
#include <transport/SecureSessionMgr.h> // For SecureSessionMgr

Expand Down
2 changes: 1 addition & 1 deletion src/app/zap-templates/templates/chip/encoder-src.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * a
PacketBufferHandle payload = PacketBuffer::NewWithAvailableSize(kMaxBufferSize); \
if (payload.IsNull()) \
{ \
ChipLogError(Zcl, "Could not allocate PacketBuffer while trying to encode %s command", kName); \
ChipLogError(Zcl, "Could not allocate packet buffer while trying to encode %s command", kName); \
return payload; \
} \
\
Expand Down
25 changes: 9 additions & 16 deletions src/ble/BLEEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ BLE_ERROR BLEEndPoint::StartConnect()
mState = kState_Connecting;

// Build BLE transport protocol capabilities request.
buf = PacketBuffer::New();
buf = System::PacketBuffer::New();
VerifyOrExit(!buf.IsNull(), err = BLE_ERROR_NO_MEMORY);

// Zero-initialize BLE transport capabilities request.
Expand All @@ -140,9 +140,8 @@ BLE_ERROR BLEEndPoint::StartConnect()

// Send BLE transport capabilities request to peripheral via GATT write.
// Add reference to message fragment for duration of platform's GATT write attempt. CHIP retains partial
// ownership of message fragment's PacketBuffer, since this is the same buffer as that of the whole message, just
// ownership of message fragment's packet buffer, since this is the same buffer as that of the whole message, just
// with a fragmenter-modified payload offset and data length, by a Retain() on the handle when calling this function.
// Buffer must be decref'd (i.e. PacketBuffer::Free'd) by platform when BLE GATT operation completes.
if (!SendWrite(buf.Retain()))
{
err = BLE_ERROR_GATT_WRITE_FAILED;
Expand Down Expand Up @@ -224,9 +223,8 @@ void BLEEndPoint::HandleSubscribeReceived()
VerifyOrExit(mBtpEngine.PopPacketTag(mSendQueue) == kType_Data, err = BLE_ERROR_INVALID_BTP_HEADER_FLAGS);
#endif
// Add reference to message fragment for duration of platform's GATT indication attempt. CHIP retains partial
// ownership of message fragment's PacketBuffer, since this is the same buffer as that of the whole message, just
// with a fragmenter-modified payload offset and data length. Buffer must be decref'd (i.e. PacketBuffer::Free'd) by
// platform when BLE GATT operation completes.
// ownership of message fragment's packet buffer, since this is the same buffer as that of the whole message, just
// with a fragmenter-modified payload offset and data length.
if (!SendIndication(mSendQueue.Retain()))
{
// Ensure transmit queue is empty and set to NULL.
Expand Down Expand Up @@ -380,11 +378,6 @@ void BLEEndPoint::FinalizeClose(uint8_t oldState, uint8_t flags, BLE_ERROR err)
mSendQueue = nullptr;
QueueTxUnlock();

#if CHIP_ENABLE_CHIPOBLE_TEST
PacketBuffer::Free(mBtpEngineTest.mCommandReceiveQueue);
mBtpEngineTest.mCommandReceiveQueue = NULL;
#endif

// Fire application's close callback if we haven't already, and it's not suppressed.
if (oldState != kState_Closing && (flags & kBleCloseFlag_SuppressCallback) == 0)
{
Expand Down Expand Up @@ -660,7 +653,7 @@ BLE_ERROR BLEEndPoint::Send(PacketBufferHandle data)
VerifyOrExit(!data.IsNull(), err = BLE_ERROR_BAD_ARGS);
VerifyOrExit(IsConnected(mState), err = BLE_ERROR_INCORRECT_STATE);

// Ensure outgoing message fits in a single contiguous PacketBuffer, as currently required by the
// Ensure outgoing message fits in a single contiguous packet buffer, as currently required by the
// message fragmentation and reassembly engine.
if (data->HasChainedBuffer())
{
Expand Down Expand Up @@ -886,7 +879,7 @@ BLE_ERROR BLEEndPoint::HandleFragmentConfirmationReceived()
// Ensure we're in correct state to receive confirmation of non-handshake GATT send.
VerifyOrExit(IsConnected(mState), err = BLE_ERROR_INCORRECT_STATE);

// TODO PacketBuffer high water mark optimization: if ack pending, but fragmenter state == complete, free fragmenter's
// TODO Packet buffer high water mark optimization: if ack pending, but fragmenter state == complete, free fragmenter's
// tx buf before sending ack.

if (GetFlag(mConnStateFlags, kConnState_StandAloneAckInFlight))
Expand Down Expand Up @@ -952,7 +945,7 @@ BLE_ERROR BLEEndPoint::DriveStandAloneAck()
// If stand-alone ack not already pending, allocate new payload buffer here.
if (mAckToSend.IsNull())
{
mAckToSend = PacketBuffer::New();
mAckToSend = System::PacketBuffer::New();
VerifyOrExit(!mAckToSend.IsNull(), err = BLE_ERROR_NO_MEMORY);
}

Expand Down Expand Up @@ -1094,7 +1087,7 @@ BLE_ERROR BLEEndPoint::HandleCapabilitiesRequestReceived(PacketBufferHandle data
err = BleTransportCapabilitiesRequestMessage::Decode(data, req);
SuccessOrExit(err);

responseBuf = PacketBuffer::New();
responseBuf = System::PacketBuffer::New();
VerifyOrExit(!responseBuf.IsNull(), err = BLE_ERROR_NO_MEMORY);

// Determine BLE connection's negotiated ATT MTU, if possible.
Expand Down Expand Up @@ -1417,7 +1410,7 @@ BLE_ERROR BLEEndPoint::Receive(PacketBufferHandle data)
// If we've reassembled a whole message...
if (mBtpEngine.RxState() == BtpEngine::kState_Complete)
{
// Take ownership of message PacketBuffer
// Take ownership of message buffer
System::PacketBufferHandle full_packet = mBtpEngine.TakeRxPacket();

ChipLogDebugBleEndPoint(Ble, "reassembled whole msg, len = %d", full_packet->DataLength());
Expand Down
10 changes: 4 additions & 6 deletions src/ble/BleLayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -267,12 +267,10 @@ class DLL_EXPORT BleLayer
*
* Beyond each call, no guarantees are provided as to the lifetime of UUID arguments.
*
* A 'true' return value means the CHIP stack successfully handled the
* corresponding message or state indication. 'false' means the CHIP stack either
* failed or chose not to handle this. In case of 'false,' the CHIP stack will not
* have freed or taken ownership of any PacketBuffer arguments. This contract allows the
* platform to pass BLE events to CHIP without needing to know which characteristics
* CHIP cares about.
* A 'true' return value means the CHIP stack successfully handled the corresponding message
* or state indication. 'false' means the CHIP stack either failed or chose not to handle this.
* This contract allows the platform to pass BLE events to CHIP without needing to know which
* characteristics CHIP cares about.
* Platform must call this function when a GATT subscription has been established to any CHIP service
* charateristic.
Expand Down
13 changes: 0 additions & 13 deletions src/ble/BlePlatformDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,22 +58,9 @@ class DLL_EXPORT BlePlatformDelegate
virtual uint16_t GetMTU(BLE_CONNECTION_OBJECT connObj) const = 0;

// Data path calling convention:
// The CHIP stack retains partial ownership of pBufs sent via the below functions. These buffers are freed by
// CHIP after either they're acknowledged by the peer's BLE controller, or CHIP shuts down the pBuf's
// associated BLEEndPoint.
//
// For its part, the platform MUST call PacketBuffer::Free on each pBuf it receives via a Send* function once it no
// longer requires a reference to this buffer, e.g. when a CHIP_CLIENT_EVENT_BLE_PBUF_CLEAR event is received on
// platforms with the CHIP BLE SDK.
//
// On platforms such as iOS or Android where the contents of the pBuf PacketBuffer are copied into a separate
// buffer for transmission, pBuf may be freed on the downcall to the platform delegate once the copy completes.
//
// A 'true' return value from a Send* function indicates that the characteristic was written or updated
// successfully. A 'false' value indicates failure, and is used to report this failure to the user via the return
// value of chipConnection::SendMessage.
//
// If a Send* function returns false, it must release its reference to pBuf prior to return.

// Send GATT characteristic indication request
virtual bool SendIndication(BLE_CONNECTION_OBJECT connObj, const ChipBleUUID * svcId, const ChipBleUUID * charId,
Expand Down
4 changes: 2 additions & 2 deletions src/ble/BtpEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ BLE_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle dat
data->ConsumeHead(startReader.OctetsRead());

// Create a new buffer for use as the Rx re-assembly area.
mRxBuf = PacketBuffer::New();
mRxBuf = System::PacketBuffer::New();

VerifyOrExit(!mRxBuf.IsNull(), err = BLE_ERROR_NO_MEMORY);

Expand Down Expand Up @@ -367,7 +367,7 @@ BLE_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle dat

if (rx_flags & kHeaderFlag_EndMessage)
{
// Trim remainder, if any, of received PacketBuffer based on sender-specified length of reassembled message.
// Trim remainder, if any, of the received packet buffer based on sender-specified length of reassembled message.
int padding = mRxBuf->DataLength() - mRxLength;

if (padding > 0)
Expand Down
1 change: 0 additions & 1 deletion src/ble/BtpEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
namespace chip {
namespace Ble {

using ::chip::System::PacketBuffer;
using ::chip::System::PacketBufferHandle;

typedef uint8_t SequenceNumber_t; // If type changed from uint8_t, adjust assumptions in BtpEngine::IsValidAck and
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ bool DeviceController_BlePlatformDelegate::SendWriteRequest(BLE_CONNECTION_OBJEC

// Going out of scope releases delegate's reference to pBuf. pBuf will be freed when both platform delegate and Chip stack free
// their references to it. We release pBuf's reference here since its payload bytes were copied into a new NSData object by
// ChipBleMgr.py's writeCB, and in both the error and succees cases this code has no further use for the pBuf PacketBuffer.
// ChipBleMgr.py's writeCB, and in both the error and succees cases this code has no further use for it.
return ret;
}

Expand Down
13 changes: 5 additions & 8 deletions src/inet/IPEndPointBasis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,6 @@
namespace chip {
namespace Inet {

using chip::System::PacketBuffer;

#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
union PeerSockAddr
{
Expand Down Expand Up @@ -678,12 +676,11 @@ IPPacketInfo * IPEndPointBasis::GetPacketInfo(const System::PacketBufferHandle &
System::Error IPEndPointBasis::PostPacketBufferEvent(chip::System::Layer & aLayer, System::Object & aTarget,
System::EventType aEventType, System::PacketBufferHandle aBuffer)
{
System::PacketBuffer * buf = aBuffer.Release_ForNow();
System::Error error = aLayer.PostEvent(aTarget, aEventType, (uintptr_t) buf);
if (error != INET_NO_ERROR)
System::Error error = aLayer.PostEvent(aTarget, aEventType, (uintptr_t) aBuffer.UnsafeGetLwIPpbuf());
if (error == INET_NO_ERROR)
{
// If PostEvent() failed, it has not taken ownership of the buffer, so we need to retake it so that it will be freed.
(void) System::PacketBufferHandle::Adopt(buf);
// If PostEvent() succeeded, it has ownership of the buffer, so we need to release it (without freeing it).
static_cast<void>(std::move(aBuffer).UnsafeRelease());
}
return error;
}
Expand Down Expand Up @@ -1070,7 +1067,7 @@ void IPEndPointBasis::HandlePendingIO(uint16_t aPort)
lPacketInfo.Clear();
lPacketInfo.DestPort = aPort;

lBuffer = PacketBuffer::New(0);
lBuffer = System::PacketBuffer::New(0);

if (!lBuffer.IsNull())
{
Expand Down
8 changes: 3 additions & 5 deletions src/inet/RawEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@
namespace chip {
namespace Inet {

using chip::System::PacketBuffer;

chip::System::ObjectPool<RawEndPoint, INET_CONFIG_NUM_RAW_ENDPOINTS> RawEndPoint::sPool;

#if CHIP_SYSTEM_CONFIG_USE_LWIP
Expand Down Expand Up @@ -623,20 +621,20 @@ INET_ERROR RawEndPoint::SendMsg(const IPPacketInfo * pktInfo, chip::System::Pack
#if LWIP_VERSION_MAJOR > 1 || LWIP_VERSION_MINOR >= 5
ip_addr_t ipAddr = addr.ToLwIPAddr();

lwipErr = raw_sendto(mRaw, msg.GetLwIPpbuf(), &ipAddr);
lwipErr = raw_sendto(mRaw, msg.UnsafeGetLwIPpbuf(), &ipAddr);
#else // LWIP_VERSION_MAJOR <= 1 && LWIP_VERSION_MINOR < 5
if (PCB_ISIPV6(mRaw))
{
ip6_addr_t ipv6Addr = addr.ToIPv6();

lwipErr = raw_sendto_ip6(mRaw, msg.GetLwIPpbuf(), &ipv6Addr);
lwipErr = raw_sendto_ip6(mRaw, msg.UnsafeGetLwIPpbuf(), &ipv6Addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

}
#if INET_CONFIG_ENABLE_IPV4
else
{
ip4_addr_t ipv4Addr = addr.ToIPv4();

lwipErr = raw_sendto(mRaw, msg.GetLwIPpbuf(), &ipv4Addr);
lwipErr = raw_sendto(mRaw, msg.UnsafeGetLwIPpbuf(), &ipv4Addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

}
#endif // INET_CONFIG_ENABLE_IPV4
#endif // LWIP_VERSION_MAJOR <= 1 || LWIP_VERSION_MINOR >= 5
Expand Down
6 changes: 2 additions & 4 deletions src/inet/TCPEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,6 @@ err_t start_tcp_timers(void)
namespace chip {
namespace Inet {

using chip::System::PacketBuffer;

chip::System::ObjectPool<TCPEndPoint, INET_CONFIG_NUM_TCP_ENDPOINTS> TCPEndPoint::sPool;

INET_ERROR TCPEndPoint::Bind(IPAddressType addrType, const IPAddress & addr, uint16_t port, bool reuseAddr)
Expand Down Expand Up @@ -2404,12 +2402,12 @@ void TCPEndPoint::ReceiveData()
bool isNewBuf = true;

if (mRcvQueue.IsNull())
rcvBuf = PacketBuffer::New(0);
rcvBuf = System::PacketBuffer::New(0);
else
{
rcvBuf = mRcvQueue->Last();
if (rcvBuf->AvailableDataLength() == 0)
rcvBuf = PacketBuffer::New(0);
rcvBuf = System::PacketBuffer::New(0);
else
{
isNewBuf = false;
Expand Down
2 changes: 1 addition & 1 deletion src/inet/TCPEndPoint.h
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis
* This method may only be called by data reception event handlers to
* put an unacknowledged portion of data back on the receive queue. The
* operational semantics are undefined if the caller is outside the scope
* of a data reception event handler, \c data is not the \c chip::System::PacketBuffer
* of a data reception event handler, \c data is not the packet buffer
* provided to the handler, or \c data does not contain the unacknowledged
* portion remaining after the bytes acknowledged by a prior call to the
* <tt>AckReceive(uint16_t len)</tt> method.
Expand Down
14 changes: 6 additions & 8 deletions src/inet/UDPEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@
namespace chip {
namespace Inet {

using chip::System::PacketBuffer;

chip::System::ObjectPool<UDPEndPoint, INET_CONFIG_NUM_UDP_ENDPOINTS> UDPEndPoint::sPool;

#if CHIP_SYSTEM_CONFIG_USE_LWIP
Expand Down Expand Up @@ -578,9 +576,9 @@ INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBuff
}

if (intfId != INET_NULL_INTERFACEID)
lwipErr = udp_sendto_if(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
lwipErr = udp_sendto_if(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
else
lwipErr = udp_sendto(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort);
lwipErr = udp_sendto(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort);

ip_addr_copy(mUDP->local_ip, boundAddr);

Expand All @@ -600,9 +598,9 @@ INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBuff
}

if (intfId != INET_NULL_INTERFACEID)
lwipErr = udp_sendto_if_ip6(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
lwipErr = udp_sendto_if_ip6(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

else
lwipErr = udp_sendto_ip6(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort);
lwipErr = udp_sendto_ip6(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

}

#if INET_CONFIG_ENABLE_IPV4
Expand All @@ -619,9 +617,9 @@ INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBuff
}

if (intfId != INET_NULL_INTERFACEID)
lwipErr = udp_sendto_if(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
lwipErr = udp_sendto_if(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort, intfId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

else
lwipErr = udp_sendto(mUDP, msg.GetLwIPpbuf(), &lwipDestAddr, destPort);
lwipErr = udp_sendto(mUDP, msg.UnsafeGetLwIPpbuf(), &lwipDestAddr, destPort);
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to use LwIPPacketBufferView

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and thanks for catching that. Looks like I rushed that change and stopped when the active builds worked.

}

ipX_addr_copy(mUDP->local_ip, boundAddr);
Expand Down
2 changes: 0 additions & 2 deletions src/inet/tests/TestInetEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -539,8 +539,6 @@ static const nlTest sTests[] = { NL_TEST_DEF("InetEndPoint::PreTest", TestInetPr
#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
/**
* Set up the test suite.
* This is a work-around to initiate PacketBuffer protected class instance's
* data and set it to a known state, before an instance is created.
*/
static int TestSetup(void * inContext)
{
Expand Down
Loading