IVGCVSW-3939 Code refactoring and minor fixes
* Fixed value masking in SendPeriodicCounterCapturePacket and updated
the pertinent unit tests
* Code refactoring and cleanup
* Added extra comments to the ProfilingService stop/reset procedure
Signed-off-by: Matteo Martincigh <matteo.martincigh@arm.com>
Change-Id: Ibaf2fede76e06d5b8ce7258a4820a60e5993559f
diff --git a/src/profiling/PeriodicCounterCapture.cpp b/src/profiling/PeriodicCounterCapture.cpp
index 5ba1318..f888bc0 100644
--- a/src/profiling/PeriodicCounterCapture.cpp
+++ b/src/profiling/PeriodicCounterCapture.cpp
@@ -30,9 +30,7 @@
m_KeepRunning.store(true);
// Start the new capture thread.
- m_PeriodCaptureThread = std::thread(&PeriodicCounterCapture::Capture,
- this,
- std::ref(m_ReadCounterValues));
+ m_PeriodCaptureThread = std::thread(&PeriodicCounterCapture::Capture, this, std::ref(m_ReadCounterValues));
}
void PeriodicCounterCapture::Stop()
@@ -47,6 +45,7 @@
m_PeriodCaptureThread.join();
}
+ // Mark the capture thread as not running
m_IsRunning = false;
}
@@ -114,7 +113,6 @@
}
while (m_KeepRunning.load());
-
}
} // namespace profiling
diff --git a/src/profiling/ProfilingService.cpp b/src/profiling/ProfilingService.cpp
index b87773f..1cc9262 100644
--- a/src/profiling/ProfilingService.cpp
+++ b/src/profiling/ProfilingService.cpp
@@ -313,8 +313,9 @@
void ProfilingService::Reset()
{
- // Reset the profiling service
+ // Stop the profiling service...
Stop();
+
// ...then delete all the counter data and configuration...
m_CounterIndex.clear();
m_CounterValues.clear();
@@ -333,13 +334,14 @@
m_SendCounterPacket.Stop(false);
m_PeriodicCounterCapture.Stop();
- // ...then destroy the profiling connection...
+ // ...then close and destroy the profiling connection...
if (m_ProfilingConnection != nullptr && m_ProfilingConnection->IsOpen())
{
m_ProfilingConnection->Close();
}
m_ProfilingConnection.reset();
+ // ...then move to the "NotConnected" state
m_StateMachine.TransitionToState(ProfilingState::NotConnected);
}
diff --git a/src/profiling/SendCounterPacket.cpp b/src/profiling/SendCounterPacket.cpp
index 41adf37..0ac6ecf 100644
--- a/src/profiling/SendCounterPacket.cpp
+++ b/src/profiling/SendCounterPacket.cpp
@@ -811,12 +811,15 @@
void SendCounterPacket::SendPeriodicCounterCapturePacket(uint64_t timestamp, const IndexValuePairsVector& values)
{
+ uint32_t uint16_t_size = sizeof(uint16_t);
+ uint32_t uint32_t_size = sizeof(uint32_t);
+ uint32_t uint64_t_size = sizeof(uint64_t);
+
uint32_t packetFamily = 1;
uint32_t packetClass = 0;
uint32_t packetType = 0;
- uint32_t headerSize = numeric_cast<uint32_t>(2 * sizeof(uint32_t));
- uint32_t bodySize = numeric_cast<uint32_t>((1 * sizeof(uint64_t)) +
- (values.size() * (sizeof(uint16_t) + sizeof(uint32_t))));
+ uint32_t headerSize = 2 * uint32_t_size;
+ uint32_t bodySize = uint64_t_size + numeric_cast<uint32_t>(values.size()) * (uint16_t_size + uint32_t_size);
uint32_t totalSize = headerSize + bodySize;
uint32_t offset = 0;
uint32_t reserved = 0;
@@ -833,22 +836,24 @@
// Create header.
WriteUint32(writeBuffer,
offset,
- ((packetFamily & 0x3F) << 26) | ((packetClass & 0x3FF) << 19) | ((packetType & 0x3FFF) << 16));
- offset += numeric_cast<uint32_t>(sizeof(uint32_t));
+ ((packetFamily & 0x0000003F) << 26) |
+ ((packetClass & 0x0000007F) << 19) |
+ ((packetType & 0x00000007) << 16));
+ offset += uint32_t_size;
WriteUint32(writeBuffer, offset, bodySize);
// Copy captured Timestamp.
- offset += numeric_cast<uint32_t>(sizeof(uint32_t));
+ offset += uint32_t_size;
WriteUint64(writeBuffer, offset, timestamp);
// Copy selectedCounterIds.
- offset += numeric_cast<uint32_t>(sizeof(uint64_t));
+ offset += uint64_t_size;
for (const auto& pair: values)
{
WriteUint16(writeBuffer, offset, pair.first);
- offset += numeric_cast<uint32_t>(sizeof(uint16_t));
+ offset += uint16_t_size;
WriteUint32(writeBuffer, offset, pair.second);
- offset += numeric_cast<uint32_t>(sizeof(uint32_t));
+ offset += uint32_t_size;
}
m_BufferManager.Commit(writeBuffer, totalSize);
@@ -857,11 +862,13 @@
void SendCounterPacket::SendPeriodicCounterSelectionPacket(uint32_t capturePeriod,
const std::vector<uint16_t>& selectedCounterIds)
{
+ uint32_t uint16_t_size = sizeof(uint16_t);
+ uint32_t uint32_t_size = sizeof(uint32_t);
+
uint32_t packetFamily = 0;
uint32_t packetId = 4;
- uint32_t headerSize = numeric_cast<uint32_t>(2 * sizeof(uint32_t));
- uint32_t bodySize = numeric_cast<uint32_t>((1 * sizeof(uint32_t)) +
- (selectedCounterIds.size() * sizeof(uint16_t)));
+ uint32_t headerSize = 2 * uint32_t_size;
+ uint32_t bodySize = uint32_t_size + numeric_cast<uint32_t>(selectedCounterIds.size()) * uint16_t_size;
uint32_t totalSize = headerSize + bodySize;
uint32_t offset = 0;
uint32_t reserved = 0;
@@ -877,19 +884,19 @@
// Create header.
WriteUint32(writeBuffer, offset, ((packetFamily & 0x3F) << 26) | ((packetId & 0x3FF) << 16));
- offset += numeric_cast<uint32_t>(sizeof(uint32_t));
+ offset += uint32_t_size;
WriteUint32(writeBuffer, offset, bodySize);
// Copy capturePeriod.
- offset += numeric_cast<uint32_t>(sizeof(uint32_t));
+ offset += uint32_t_size;
WriteUint32(writeBuffer, offset, capturePeriod);
// Copy selectedCounterIds.
- offset += numeric_cast<uint32_t>(sizeof(uint32_t));
+ offset += uint32_t_size;
for(const uint16_t& id: selectedCounterIds)
{
WriteUint16(writeBuffer, offset, id);
- offset += numeric_cast<uint32_t>(sizeof(uint16_t));
+ offset += uint16_t_size;
}
m_BufferManager.Commit(writeBuffer, totalSize);
diff --git a/src/profiling/SocketProfilingConnection.cpp b/src/profiling/SocketProfilingConnection.cpp
index 6f7101b..2c4ff76 100644
--- a/src/profiling/SocketProfilingConnection.cpp
+++ b/src/profiling/SocketProfilingConnection.cpp
@@ -78,44 +78,42 @@
Packet SocketProfilingConnection::ReadPacket(uint32_t timeout)
{
- // Is there currently at least a headers worth of data waiting to be read?
- int bytes_available;
+ // Is there currently at least a header worth of data waiting to be read?
+ int bytes_available = 0;
ioctl(m_Socket[0].fd, FIONREAD, &bytes_available);
if (bytes_available >= 8)
{
// Yes there is. Read it:
return ReceivePacket();
}
- else
+
+ // Poll for data on the socket or until timeout occurs
+ int pollResult = poll(m_Socket, 1, static_cast<int>(timeout));
+
+ switch (pollResult)
{
- // Poll for data on the socket or until timeout occurs
- int pollResult = poll(m_Socket, 1, static_cast<int>(timeout));
+ case -1: // Error
+ throw armnn::RuntimeException(std::string("Read failure from socket: ") + strerror(errno));
- switch (pollResult)
+ case 0: // Timeout
+ throw TimeoutException("Timeout while reading from socket");
+
+ default: // Normal poll return but it could still contain an error signal
+
+ // Check if the socket reported an error
+ if (m_Socket[0].revents & (POLLNVAL | POLLERR | POLLHUP))
{
- case -1: // Error
- throw armnn::RuntimeException(std::string("Read failure from socket: ") + strerror(errno));
-
- case 0: // Timeout
- throw TimeoutException("Timeout while reading from socket");
-
- default: // Normal poll return but it could still contain an error signal
-
- // Check if the socket reported an error
- if (m_Socket[0].revents & (POLLNVAL | POLLERR | POLLHUP))
- {
- throw armnn::RuntimeException(std::string("Socket reported an error: ") + strerror(errno));
- }
-
- // Check if there is data to read
- if (!(m_Socket[0].revents & (POLLIN)))
- {
- // This is a very odd case. The file descriptor was woken up but no data was written.
- throw armnn::RuntimeException("Poll resulted in : no data to read");
- }
-
- return ReceivePacket();
+ throw armnn::RuntimeException(std::string("Socket reported an error: ") + strerror(errno));
}
+
+ // Check if there is data to read
+ if (!(m_Socket[0].revents & (POLLIN)))
+ {
+ // This is a very odd case. The file descriptor was woken up but no data was written.
+ throw armnn::RuntimeException("Poll resulted in : no data to read");
+ }
+
+ return ReceivePacket();
}
}
@@ -127,6 +125,7 @@
// What do we do here if there's not a valid 8 byte header to read?
throw armnn::RuntimeException("The received packet did not contains a valid MIPE header");
}
+
// stream_metadata_identifier is the first 4 bytes
uint32_t metadataIdentifier = 0;
std::memcpy(&metadataIdentifier, header, sizeof(metadataIdentifier));
@@ -135,10 +134,10 @@
uint32_t dataLength = 0;
std::memcpy(&dataLength, header + 4u, sizeof(dataLength));
- std::unique_ptr<unsigned char[]> packetData;
+ std::unique_ptr<unsigned char[]> packetData;
if (dataLength > 0)
{
- packetData = std::make_unique<unsigned char[]>(dataLength);
+ packetData = std::make_unique<unsigned char[]>(dataLength);
ssize_t receivedLength = recv(m_Socket[0].fd, packetData.get(), dataLength, 0);
if (receivedLength < 0)
{
diff --git a/src/profiling/test/ProfilingTests.cpp b/src/profiling/test/ProfilingTests.cpp
index b32a55c..50af75e 100644
--- a/src/profiling/test/ProfilingTests.cpp
+++ b/src/profiling/test/ProfilingTests.cpp
@@ -2119,10 +2119,10 @@
uint32_t headerWord0 = ReadUint32(buffer, 0);
uint32_t headerWord1 = ReadUint32(buffer, 4);
- BOOST_TEST(((headerWord0 >> 26) & 0x3F) == 1); // packet family
- BOOST_TEST(((headerWord0 >> 19) & 0x3F) == 0); // packet class
- BOOST_TEST(((headerWord0 >> 16) & 0x3) == 0); // packet type
- BOOST_TEST(headerWord1 == 20); // data length
+ BOOST_TEST(((headerWord0 >> 26) & 0x0000003F) == 1); // packet family
+ BOOST_TEST(((headerWord0 >> 19) & 0x0000007F) == 0); // packet class
+ BOOST_TEST(((headerWord0 >> 16) & 0x00000007) == 0); // packet type
+ BOOST_TEST(headerWord1 == 20);
uint32_t offset = 16;
uint16_t readIndex = ReadUint16(buffer, offset);
diff --git a/src/profiling/test/SendCounterPacketTests.cpp b/src/profiling/test/SendCounterPacketTests.cpp
index 00dad38..f0ba347 100644
--- a/src/profiling/test/SendCounterPacketTests.cpp
+++ b/src/profiling/test/SendCounterPacketTests.cpp
@@ -217,11 +217,11 @@
uint32_t headerWord1 = ReadUint32(readBuffer2, 4);
uint64_t readTimestamp = ReadUint64(readBuffer2, 8);
- BOOST_TEST(((headerWord0 >> 26) & 0x3F) == 1); // packet family
- BOOST_TEST(((headerWord0 >> 19) & 0x3F) == 0); // packet class
- BOOST_TEST(((headerWord0 >> 16) & 0x3) == 0); // packet type
- BOOST_TEST(headerWord1 == 8); // data length
- BOOST_TEST(time == readTimestamp); // capture period
+ BOOST_TEST(((headerWord0 >> 26) & 0x0000003F) == 1); // packet family
+ BOOST_TEST(((headerWord0 >> 19) & 0x0000007F) == 0); // packet class
+ BOOST_TEST(((headerWord0 >> 16) & 0x00000007) == 0); // packet type
+ BOOST_TEST(headerWord1 == 8); // data length
+ BOOST_TEST(time == readTimestamp); // capture period
// Full packet message
MockBufferManager mockBuffer3(512);
@@ -240,11 +240,11 @@
headerWord1 = ReadUint32(readBuffer3, 4);
uint64_t readTimestamp2 = ReadUint64(readBuffer3, 8);
- BOOST_TEST(((headerWord0 >> 26) & 0x3F) == 1); // packet family
- BOOST_TEST(((headerWord0 >> 19) & 0x3F) == 0); // packet class
- BOOST_TEST(((headerWord0 >> 16) & 0x3) == 0); // packet type
- BOOST_TEST(headerWord1 == 38); // data length
- BOOST_TEST(time == readTimestamp2); // capture period
+ BOOST_TEST(((headerWord0 >> 26) & 0x0000003F) == 1); // packet family
+ BOOST_TEST(((headerWord0 >> 19) & 0x0000007F) == 0); // packet class
+ BOOST_TEST(((headerWord0 >> 16) & 0x00000007) == 0); // packet type
+ BOOST_TEST(headerWord1 == 38); // data length
+ BOOST_TEST(time == readTimestamp2); // capture period
uint16_t counterIndex = 0;
uint32_t counterValue = 100;