Skip to content

Commit

Permalink
Begin SystemPacketBuffer.h cleanup (#4096)
Browse files Browse the repository at this point in the history
* Begin 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

- Consolidated PacketBuffer and PacketBufferHeader method descriptions
  into the header; clarified which are in transition.
- Most uses of Next(), which returns a raw pointer, only checked
  existence; added PacketBuffer::HasChainedBuffer() to replace these.
- Removed DetachTail_ForNow(), as it has no remaining callers.
- Converted TestSystemPacketBuffer to friended class, so that it can
  continue to use and test private methods. (Refactoring to focus on the
  PacketBufferHandle interface will follow.)

Part of issue #2707 - Figure out a way to express PacketBuffer ownership in the type system

* Fix Doxygen

* Fix Doxygen ONCE AND FOR ALL

* Fix description

* Description proofreading
  • Loading branch information
kpschoedel authored Dec 8, 2020
1 parent eb8217f commit 548b71a
Show file tree
Hide file tree
Showing 10 changed files with 497 additions and 319 deletions.
6 changes: 2 additions & 4 deletions src/ble/BLEEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -662,11 +662,11 @@ BLE_ERROR BLEEndPoint::Send(PacketBufferHandle data)

// Ensure outgoing message fits in a single contiguous PacketBuffer, as currently required by the
// message fragmentation and reassembly engine.
if (data->Next() != nullptr)
if (data->HasChainedBuffer())
{
data->CompactHead();

if (data->Next() != nullptr)
if (data->HasChainedBuffer())
{
err = BLE_ERROR_OUTBOUND_MESSAGE_TOO_BIG;
ExitNow();
Expand Down Expand Up @@ -1056,7 +1056,6 @@ BLE_ERROR BLEEndPoint::DriveSending()
#if CHIP_ENABLE_CHIPOBLE_TEST
mBtpEngineTest.DoTxTiming(sentBuf, BTP_TX_DONE);
#endif // CHIP_ENABLE_CHIPOBLE_TEST
mBtpEngine.ClearTxPacket();

if (!mSendQueue.IsNull())
{
Expand Down Expand Up @@ -1420,7 +1419,6 @@ BLE_ERROR BLEEndPoint::Receive(PacketBufferHandle data)
{
// Take ownership of message PacketBuffer
System::PacketBufferHandle full_packet = mBtpEngine.TakeRxPacket();
mBtpEngine.ClearRxPacket();

ChipLogDebugBleEndPoint(Ble, "reassembled whole msg, len = %d", full_packet->DataLength());

Expand Down
10 changes: 5 additions & 5 deletions src/ble/BtpEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ BLE_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle dat

// For now, limit BtpEngine message size to max length of 1 pbuf, as we do for chip messages sent via IP.
// TODO add support for BtpEngine messages longer than 1 pbuf
VerifyOrExit(mRxBuf->Next() == nullptr, err = BLE_ERROR_RECEIVED_MESSAGE_TOO_BIG);
VerifyOrExit(!mRxBuf->HasChainedBuffer(), err = BLE_ERROR_RECEIVED_MESSAGE_TOO_BIG);
}
else
{
Expand Down Expand Up @@ -417,13 +417,13 @@ BLE_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle dat
return err;
}

void BtpEngine::ClearRxPacket()
PacketBufferHandle BtpEngine::TakeRxPacket()
{
if (mRxState == kState_Complete)
{
mRxState = kState_Idle;
}
mRxBuf = nullptr;
return std::move(mRxBuf);
}

// Calling convention:
Expand Down Expand Up @@ -570,13 +570,13 @@ bool BtpEngine::HandleCharacteristicSend(System::PacketBufferHandle data, bool s
return true;
}

void BtpEngine::ClearTxPacket()
PacketBufferHandle BtpEngine::TakeTxPacket()
{
if (mTxState == kState_Complete)
{
mTxState = kState_Idle;
}
mTxBuf = nullptr;
return std::move(mTxBuf);
}

void BtpEngine::LogState() const
Expand Down
8 changes: 4 additions & 4 deletions src/ble/BtpEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,12 @@ class BtpEngine
bool HandleCharacteristicSend(System::PacketBufferHandle data, bool send_ack);
BLE_ERROR EncodeStandAloneAck(const PacketBufferHandle & data);

void ClearRxPacket();
PacketBufferHandle TakeRxPacket() { return std::move(mRxBuf); }
PacketBufferHandle TakeRxPacket();
PacketBufferHandle BorrowRxPacket() { return mRxBuf.Retain(); }
void ClearTxPacket();
PacketBufferHandle TakeTxPacket() { return std::move(mTxBuf); }
void ClearRxPacket() { (void) TakeRxPacket(); }
PacketBufferHandle TakeTxPacket();
PacketBufferHandle BorrowTxPacket() { return mTxBuf.Retain(); }
void ClearTxPacket() { (void) TakeTxPacket(); }

void LogState() const;
void LogStateDebug() const;
Expand Down
2 changes: 1 addition & 1 deletion src/inet/IPEndPointBasis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ INET_ERROR IPEndPointBasis::SendMsg(const IPPacketInfo * aPktInfo, chip::System:
VerifyOrExit(mAddrType == aPktInfo->DestAddress.Type(), res = INET_ERROR_BAD_ARGS);

// For now the entire message must fit within a single buffer.
VerifyOrExit(aBuffer->Next() == nullptr, res = INET_ERROR_MESSAGE_TOO_LONG);
VerifyOrExit(!aBuffer->HasChainedBuffer(), res = INET_ERROR_MESSAGE_TOO_LONG);

memset(&msgHeader, 0, sizeof(msgHeader));

Expand Down
Loading

0 comments on commit 548b71a

Please sign in to comment.