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

6 files changed, 69 insertions, 63 deletions

diff --git a/src/profiling/PeriodicCounterCapture.cpp b/src/profiling/PeriodicCounterCapture.cpp
index 5ba1318a77..f888bc045e 100644
--- a/src/profiling/PeriodicCounterCapture.cpp
+++ b/src/profiling/PeriodicCounterCapture.cpp
@@ -30,9 +30,7 @@ void PeriodicCounterCapture::Start()
     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 @@ void PeriodicCounterCapture::Stop()
         m_PeriodCaptureThread.join();
     }
 
+    // Mark the capture thread as not running
     m_IsRunning = false;
 }
 
@@ -114,7 +113,6 @@ void PeriodicCounterCapture::Capture(const IReadCounterValues& readCounterValues
 
     }
     while (m_KeepRunning.load());
-
 }
 
 } // namespace profiling
diff --git a/src/profiling/ProfilingService.cpp b/src/profiling/ProfilingService.cpp
index b87773fc86..1cc9262420 100644
--- a/src/profiling/ProfilingService.cpp
+++ b/src/profiling/ProfilingService.cpp
@@ -313,8 +313,9 @@ void ProfilingService::InitializeCounterValue(uint16_t counterUid)
 
 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 @@ void ProfilingService::Stop()
     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 41adf37244..0ac6ecfca3 100644
--- a/src/profiling/SendCounterPacket.cpp
+++ b/src/profiling/SendCounterPacket.cpp
@@ -811,12 +811,15 @@ void SendCounterPacket::SendCounterDirectoryPacket(const ICounterDirectory& coun
 
 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 @@ void SendCounterPacket::SendPeriodicCounterCapturePacket(uint64_t timestamp, con
     // 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::SendPeriodicCounterCapturePacket(uint64_t timestamp, con
 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 @@ void SendCounterPacket::SendPeriodicCounterSelectionPacket(uint32_t capturePerio
 
     // 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 6f7101b254..2c4ff76dc8 100644
--- a/src/profiling/SocketProfilingConnection.cpp
+++ b/src/profiling/SocketProfilingConnection.cpp
@@ -78,44 +78,42 @@ bool SocketProfilingConnection::WritePacket(const unsigned char* buffer, uint32_
 
 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)
-        {
-            case -1: // Error
-                throw armnn::RuntimeException(std::string("Read failure from socket: ") + strerror(errno));
+    // Poll for data on the socket or until timeout occurs
+    int pollResult = poll(m_Socket, 1, static_cast<int>(timeout));
 
-            case 0: // Timeout
-                throw TimeoutException("Timeout while reading from socket");
+    switch (pollResult)
+    {
+    case -1: // Error
+        throw armnn::RuntimeException(std::string("Read failure from socket: ") + strerror(errno));
 
-            default: // Normal poll return but it could still contain an error signal
+    case 0: // Timeout
+        throw TimeoutException("Timeout while reading from socket");
 
-                // 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));
-                }
+    default: // Normal poll return but it could still contain an error signal
 
-                // 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");
-                }
+        // 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));
+        }
 
-                return ReceivePacket();
+        // 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 @@ Packet SocketProfilingConnection::ReceivePacket()
         // 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 @@ Packet SocketProfilingConnection::ReceivePacket()
     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 b32a55cc6d..50af75eeea 100644
--- a/src/profiling/test/ProfilingTests.cpp
+++ b/src/profiling/test/ProfilingTests.cpp
@@ -2119,10 +2119,10 @@ BOOST_AUTO_TEST_CASE(CheckPeriodicCounterCaptureThread)
     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 00dad38078..f0ba34778c 100644
--- a/src/profiling/test/SendCounterPacketTests.cpp
+++ b/src/profiling/test/SendCounterPacketTests.cpp
@@ -217,11 +217,11 @@ BOOST_AUTO_TEST_CASE(SendPeriodicCounterCapturePacketTest)
     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 @@ BOOST_AUTO_TEST_CASE(SendPeriodicCounterCapturePacketTest)
     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;