Unverified Commit 807d64ed authored by Ken A. Redergård's avatar Ken A. Redergård Committed by GitHub
Browse files

Fix H5Transport timeout issues (#116)

* Fix H5Transport timeout issues

This commit addresses timeout issues that sometimes occur when calling sd_rpc_open.

The problem is identified to be in the transport layers.

The issue is addressed by:
    - Improving the state machine in H5Transport and improving UartBoost implementation
    - Requiring latest version of Boost (1.67). This to make sure all fixes in Boost ASIO serialport are included.

State machine
=============
The H5Transport is now waiting for the internal state machine to be ready before opening the serial port that sets state machine values.
The state machine transitions cover more exit criterias and the order of evaluating the exit criterias are improved.

Two new states are added: STATE_CLOSED and STATE_NO_RESPONSE. 

The state machine enters STATE_CLOSED when the user close the transport.
The state machine enters STATE_NO_RESPONSE when the device does not reply on packets sent after n number of retries.

The new states are used to give a more precise return value from sd_rpc_open.

toString methods on ExitCriterias based classes are added to simplify
debugging.

Integrations tests
==================
Integration tests for open close functionality has been added for pc-ble-driver, H5Transport and UartBoost.

The integration tests are implemented with the use of Catch2, a header file library test framework.
Initially Boost Test Library was used, but since we are considering moving away from Boost, we try not to add more Boost dependencies.

The old test that tested the Boost serial port implementation is removed.

Build system
============
The Boost library file names made by jam (boost build system) has changed for Windows builds.
For cmake to find the libraries one need to use a recent version of cmake.

We have tested it with cmake 3.11 and set it as a requirement.

Other changes
=============
* General changes to the source code:
    - Added const-ness where applicable
    - Where applicable, changed pointers to type pointed to or *_ptr
    - Made payload into an alias (from std::vector<uint8_t>)
    - Expose methods and functions to facilitate integration testing
    - Typedefs changes to using
    - Changed callback names to make it clearer if callback was intended for local transport or upper transport

Branch workflow
===============
We are now changing the pc-ble-driver git workflow.
Fixes and improvements that are relevant for all major versions of the driver are fixed in master then cherry picked into the major version branches.

To get to this state, this commit contains cherry picked commits from different branches.

Here is a table of cherry picked commits:
Uart boost fixes:
85cee39958f43d3fc07d596221af3b9a88b64bec
f566ce385ffbdd149274ae247f42447b5297490b
d8711bed9de7513ac68e6ca091fe6fc96ce93af9

H5Transport fixes:
e72d446f6d4de062535e8ec18b3caba8769aad32
6cfae50d4e3ae9103349e6c57de158511262d6e7

Spurious wakeup fixes and state machine fixes:
21e2556644633f5280edb9ddb88dcfabe9b2c807
parent 971241c1
This diff is collapsed.
cmake_minimum_required(VERSION 3.3)
cmake_minimum_required(VERSION 3.11)
project(pc-ble-driver)
include (cmake/pc-ble-driver.cmake)
......@@ -84,9 +85,6 @@ foreach(SD_API_VER ${SD_API_VERS})
add_library(${PC_BLE_DRIVER_${SD_API_VER}_STATIC_LIB} STATIC $<TARGET_OBJECTS:${PC_BLE_DRIVER_${SD_API_VER}_OBJ_LIB}>)
endforeach(SD_API_VER)
# build test_uart executable
add_executable(test_uart test/test_uart.cpp)
# Set common include directories
include_directories(
include/common
......@@ -131,7 +129,6 @@ foreach(SD_API_VER ${SD_API_VERS})
# Assume Linux
target_link_libraries(${PC_BLE_DRIVER_${SD_API_VER}_STATIC_LIB} PRIVATE "udev" "pthread")
target_link_libraries(${PC_BLE_DRIVER_${SD_API_VER}_SHARED_LIB} PRIVATE "udev" "pthread")
target_link_libraries(test_uart PRIVATE "pthread")
endif()
# Specify libraries to link serialization library with
......@@ -139,6 +136,5 @@ foreach(SD_API_VER ${SD_API_VERS})
target_link_libraries (${PC_BLE_DRIVER_${SD_API_VER}_STATIC_LIB} PRIVATE ${Boost_LIBRARIES})
endforeach(SD_API_VER)
target_link_libraries(test_uart PRIVATE ${Boost_LIBRARIES})
# Add tests
add_subdirectory(test)
\ No newline at end of file
......@@ -98,7 +98,7 @@ Note: This step is not required for macOS (OS X).
Use the following link to download the Boost source code:
* [Boost](http://www.boost.org/users/download) (>=1.54.0)
* [Boost](http://www.boost.org/users/download) (>=1.67.0)
- Download and extract Boost to a folder of your choice.
- Set the environment variable `BOOST_ROOT` to the path where you have extracted Boost.
......@@ -194,7 +194,7 @@ Assuming that you have built the Boost libraries and installed the tools require
To build this project you will need the following tools:
* [CMake](https://cmake.org/) (>=3.3)
* [CMake](https://cmake.org/) (>=3.11)
* A C/C++ toolchain (should already have been installed to build Boost)
See the following sections for platform-specific instructions on the installation of the dependencies.
......
......@@ -51,8 +51,8 @@ set(Boost_USE_MULTITHREADED ON)
set(Boost_USE_STATIC_LIBS ON)
# Find the necessary boost components on the system.
# Minimum version required is 1.54.0
find_package ( Boost 1.54.0 REQUIRED COMPONENTS thread system regex date_time chrono )
# Minimum version required is 1.67.0
find_package ( Boost 1.67.0 REQUIRED COMPONENTS thread system regex date_time chrono )
# Add or remove SD API versions here
set(SD_API_VER_NUMS 2 5)
......
/*
* Copyright (c) 2018 Nordic Semiconductor ASA
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without modification,
* are permitted provided that the following conditions are met:
*
* 1. Redistributions of source code must retain the above copyright notice, this
* list of conditions and the following disclaimer.
*
* 2. Redistributions in binary form must reproduce the above copyright notice, this
* list of conditions and the following disclaimer in the documentation and/or
* other materials provided with the distribution.
*
* 3. Neither the name of Nordic Semiconductor ASA nor the names of other
* contributors to this software may be used to endorse or promote products
* derived from this software without specific prior written permission.
*
* 4. This software must only be used in or with a processor manufactured by Nordic
* Semiconductor ASA, or in or with a processor manufactured by a third party that
* is used in combination with a processor manufactured by Nordic Semiconductor.
*
* 5. Any software provided in binary or object form under this license must not be
* reverse engineered, decompiled, modified and/or disassembled.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
#ifndef NRF_LOG_H__
#define NRF_LOG_H__
#include <sstream>
#include <iostream>
#include <string>
#include <fstream>
#include <mutex>
#include <chrono>
#include <iomanip>
// You should not use stdout/stderr on Windows since this will have a huge impact on
// the application since this logger is not offloading the displaying of data
// to a separate thread.
//
// This stack overflow thread contains more info in regards to cmd.exe output:
// https://stackoverflow.com/questions/7404551/why-is-console-output-so-slow
#ifndef NRF_LOG_SETUP
#ifndef NRF_LOG_FILENAME
std::ostream &nrfLogStream(std::cout); \
static std::mutex nrfLogMutex;
#else
std::fstream nrfLogStream(\
NRF_LOG_FILENAME, \
std::fstream::out | std::fstream::trunc); \
static std::mutex nrfLogMutex;
#endif
#define NRF_LOG_SETUP
#endif
#ifndef NRF_LOG
#define NRF_LOG(message) do { \
auto timestamp = std::chrono::high_resolution_clock::now(); \
auto threadID = std::this_thread::get_id(); \
std::unique_lock<std::mutex> lock(nrfLogMutex); \
std::stringstream stream; \
stream << message; \
nrfLogStream << "@" << std::right << std::setfill('0') << std::setw(10) << std::chrono::duration_cast<std::chrono::microseconds>(timestamp.time_since_epoch()).count() \
<< " [" << std::setw(5) << threadID << "] " \
<< "| " << message << std::endl << std::flush; \
} while(0);
#endif // NRF_LOG
#endif // NRF_LOG_H__
......@@ -57,15 +57,16 @@ typedef enum
typedef enum
{
CONTROL_PKT_RESET,
CONTROL_PKT_ACK,
CONTROL_PKT_SYNC,
CONTROL_PKT_SYNC_RESPONSE,
CONTROL_PKT_SYNC_CONFIG,
CONTROL_PKT_SYNC_CONFIG_RESPONSE,
CONTROL_PKT_RESET = 0,
CONTROL_PKT_ACK = 1,
CONTROL_PKT_SYNC = 2,
CONTROL_PKT_SYNC_RESPONSE = 3,
CONTROL_PKT_SYNC_CONFIG = 4,
CONTROL_PKT_SYNC_CONFIG_RESPONSE = 5,
CONTROL_PKT_LAST = 10
} control_pkt_type;
void h5_encode(std::vector<uint8_t> &in_packet,
void h5_encode(const std::vector<uint8_t> &in_packet,
std::vector<uint8_t> &out_packet,
uint8_t seq_num,
uint8_t ack_num,
......
......@@ -49,6 +49,7 @@
#include <map>
#include <thread>
#include "h5.h"
#include "h5_transport_exit_criterias.h"
typedef enum
{
......@@ -58,154 +59,37 @@ typedef enum
STATE_INITIALIZED,
STATE_ACTIVE,
STATE_FAILED,
STATE_CLOSED,
STATE_NO_RESPONSE,
STATE_UNKNOWN
} h5_state_t;
typedef std::function<h5_state_t()> state_action_t;
class ExitCriterias
{
public:
ExitCriterias() : ioResourceError(false), close(false) {}
virtual ~ExitCriterias() {}
bool ioResourceError;
bool close;
virtual bool isFullfilled() const = 0;
virtual void reset()
{
ioResourceError = false; close = false;
}
};
class StartExitCriterias : public ExitCriterias
{
public:
bool isOpened;
StartExitCriterias()
: ExitCriterias(),
isOpened(false) {}
bool isFullfilled() const override
{
return (isOpened || ioResourceError);
}
void reset() override
{
ExitCriterias::reset();
isOpened = false;
}
};
class UninitializedExitCriterias : public ExitCriterias
{
public:
bool syncSent;
bool syncRspReceived;
UninitializedExitCriterias()
: ExitCriterias(),
syncSent(false),
syncRspReceived(false) {}
bool isFullfilled() const override
{
return (syncSent && syncRspReceived) || ioResourceError || close;
}
void reset() override
{
ExitCriterias::reset();
syncSent = false;
syncRspReceived = false;
}
};
class InitializedExitCriterias : public ExitCriterias
{
public:
bool syncConfigSent;
bool syncConfigRspReceived;
InitializedExitCriterias()
: ExitCriterias(),
syncConfigSent(false),
syncConfigRspReceived(false) {}
bool isFullfilled() const override
{
return ioResourceError || close || (syncConfigSent && syncConfigRspReceived);
}
void reset() override
{
ExitCriterias::reset();
syncConfigSent = false;
syncConfigRspReceived = false;
};
};
class ActiveExitCriterias : public ExitCriterias
{
public:
bool irrecoverableSyncError;
bool syncReceived;
ActiveExitCriterias()
: ExitCriterias(), irrecoverableSyncError(false),
syncReceived(false) {}
bool isFullfilled() const override {
return ioResourceError || syncReceived || close || irrecoverableSyncError;
}
void reset() override
{
ExitCriterias::reset();
irrecoverableSyncError = false;
syncReceived = false;
close = false;
}
};
class ResetExitCriterias : public ExitCriterias
{
public:
bool resetSent;
ResetExitCriterias()
: ExitCriterias(), resetSent(false)
{}
bool isFullfilled() const override
{
return ioResourceError || close || resetSent;
}
void reset() override
{
ExitCriterias::reset();
resetSent = false;
}
};
using state_action_t = std::function<h5_state_t()>;
using payload_t = std::vector<uint8_t>;
class H5Transport : public Transport {
public:
H5Transport() = delete;
H5Transport(Transport *nextTransportLayer, uint32_t retransmission_interval);
~H5Transport();
uint32_t open(status_cb_t status_callback, data_cb_t data_callback, log_cb_t log_callback) override;
uint32_t close() override;
uint32_t send(std::vector<uint8_t> &data) override;
uint32_t send(const std::vector<uint8_t> &data) override;
h5_state_t state() const;
static bool isSyncPacket(const payload_t &packet, const uint8_t offset = 0);
static bool isSyncResponsePacket(const payload_t &packet, const uint8_t offset = 0);
static bool isSyncConfigPacket(const payload_t &packet, const uint8_t offset = 0);
static bool isSyncConfigResponsePacket(const payload_t &packet, const uint8_t offset = 0);
static bool isResetPacket(const payload_t &packet, const uint8_t offset = 0);
static bool checkPattern(const payload_t &packet, const uint8_t offset, const payload_t &pattern);
private:
void dataHandler(uint8_t *data, size_t length);
void statusHandler(sd_rpc_app_status_t code, const char * error);
void processPacket(std::vector<uint8_t>& packet);
void processPacket(payload_t &packet);
void sendControlPacket(control_pkt_type type);
......@@ -215,6 +99,11 @@ private:
Transport *nextTransportLayer;
std::vector<uint8_t> lastPacket;
// Callbacks used by lower transports
// These callbacks invoke upper callbacks
status_cb_t statusCallback;
data_cb_t dataCallback;
// Variables used for reliable packets
uint8_t seqNum;
uint8_t ackNum;
......@@ -222,9 +111,8 @@ private:
bool c0Found;
std::vector<uint8_t> unprocessedData;
// Variables used in state RESET/UNINITIALIZED/INITIALIZED
std::mutex syncMutex; // TODO: evaluate a new name for syncMutex
std::condition_variable syncWaitCondition; // TODO: evaluate a new name for syncWaitCondition
std::mutex stateMachineMutex; // Mutex controlling access to state machine variables
std::condition_variable stateMachineChange; // Condition variable to communicate changes to state machine
// Variables used in state ACTIVE
std::chrono::milliseconds retransmissionInterval;
......@@ -236,18 +124,21 @@ private:
uint32_t outgoingPacketCount;
uint32_t errorPacketCount;
void logPacket(bool outgoing, std::vector<uint8_t> &packet);
void logPacket(bool outgoing, payload_t &packet);
void log(std::string &logLine) const;
void log(char const *logLine) const;
void logStateTransition(h5_state_t from, h5_state_t to) const;
static std::string stateToString(h5_state_t state);
std::string asHex(std::vector<uint8_t> &packet) const;
std::string hciPacketLinkControlToString(std::vector<uint8_t> payload) const;
std::string h5PktToString(bool out, std::vector<uint8_t> &h5Packet) const;
std::string asHex(payload_t &packet) const;
std::string hciPacketLinkControlToString(payload_t payload) const;
std::string h5PktToString(bool out, payload_t &h5Packet) const;
static std::string pktTypeToString(h5_pkt_type_t pktType);
// State machine related
h5_state_t currentState;
std::thread stateMachineThread;
bool stateMachineReady;
std::map<h5_state_t, state_action_t> stateActions;
void setupStateMachine();
......@@ -256,27 +147,25 @@ private:
std::map<h5_state_t, ExitCriterias*> exitCriterias;
std::thread *stateMachineThread;
void stateMachineWorker();
bool runStateMachine;
std::mutex stateMutex; // Mutex that allows threads to wait for a given state in the state machine
bool waitForState(h5_state_t state, std::chrono::milliseconds timeout);
std::condition_variable stateWaitCondition;
static std::map<control_pkt_type, std::vector<uint8_t>> pkt_pattern;
static std::map<control_pkt_type, payload_t> pkt_pattern;
static std::map<h5_state_t, std::string> stateString;
static std::map<h5_pkt_type_t, std::string> pktTypeString;
static const uint8_t syncFirstByte = 0x01;
static const uint8_t syncSecondByte = 0x7E;
static const uint8_t syncRspFirstByte = 0x02;
static const uint8_t syncRspSecondByte = 0x7D;
static const uint8_t syncConfigFirstByte = 0x03;
static const uint8_t syncConfigSecondByte = 0xFC;
static const uint8_t syncConfigRspFirstByte = 0x04;
static const uint8_t syncConfigRspSecondByte = 0x7B;
static const uint8_t syncConfigField = 0x11;
static const uint8_t SyncFirstByte = 0x01;
static const uint8_t SyncSecondByte = 0x7E;
static const uint8_t SyncRspFirstByte = 0x02;
static const uint8_t SyncRspSecondByte = 0x7D;
static const uint8_t SyncConfigFirstByte = 0x03;
static const uint8_t SyncConfigSecondByte = 0xFC;
static const uint8_t SyncConfigRspFirstByte = 0x04;
static const uint8_t SyncConfigRspSecondByte = 0x7B;
static const uint8_t SyncConfigField = 0x11;
};
#endif //H5_TRANSPORT_H
/*
* Copyright (c) 2018 Nordic Semiconductor ASA
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without modification,
* are permitted provided that the following conditions are met:
*
* 1. Redistributions of source code must retain the above copyright notice, this
* list of conditions and the following disclaimer.
*
* 2. Redistributions in binary form must reproduce the above copyright notice, this
* list of conditions and the following disclaimer in the documentation and/or
* other materials provided with the distribution.
*
* 3. Neither the name of Nordic Semiconductor ASA nor the names of other
* contributors to this software may be used to endorse or promote products
* derived from this software without specific prior written permission.
*
* 4. This software must only be used in or with a processor manufactured by Nordic
* Semiconductor ASA, or in or with a processor manufactured by a third party that
* is used in combination with a processor manufactured by Nordic Semiconductor.
*
* 5. Any software provided in binary or object form under this license must not be
* reverse engineered, decompiled, modified and/or disassembled.
*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
* WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR
* ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
* (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
* LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
* ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
* SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*/
#ifndef H5_TRANSPORT_EXIT_CRITERIAS_H
#define H5_TRANSPORT_EXIT_CRITERIAS_H
#include <sstream>
class ExitCriterias
{
public:
ExitCriterias() noexcept
: ioResourceError(false), close(false) {}
ExitCriterias(const ExitCriterias&) = delete;
virtual ~ExitCriterias() {}
bool ioResourceError;
bool close;
virtual bool isFullfilled() const = 0;
virtual void reset()
{
ioResourceError = false; close = false;
}
virtual std::string toString()
{
std::stringstream info;
info << "ioResourceError:" << ioResourceError << " close:" << close;
return info.str();
}
};
class StartExitCriterias : public ExitCriterias
{
public:
bool isOpened;
StartExitCriterias() noexcept
: ExitCriterias(),
isOpened(false) {}
StartExitCriterias(const StartExitCriterias&) = delete;
bool isFullfilled() const override
{
return ioResourceError || close || isOpened;
}
void reset() override
{
ExitCriterias::reset();
isOpened = false;
}
std::string toString() override
{
std::stringstream info;
info << "state:START " << ExitCriterias::toString()
<< " isOpened:" << isOpened
<< " isFullfilled:" << isFullfilled();
return info.str();
}
};
class ResetExitCriterias : public ExitCriterias
{
public:
bool resetSent;
bool resetWait;
ResetExitCriterias() noexcept
: ExitCriterias(), resetSent(false), resetWait(false) {}
ResetExitCriterias(const ResetExitCriterias&) = delete;
bool isFullfilled() const override
{
return ioResourceError || close || (resetSent && resetWait);
}
void reset() override
{
ExitCriterias::reset();
resetSent = false;
resetWait = false;
}
std::string toString() override
{
std::stringstream info;
info << "state:RESET " << ExitCriterias::toString()
<< " resetSent:" << resetSent
<< " resetWait:" << resetWait
<< " isFullfilled:" << isFullfilled();
return info.str();
}
};
class UninitializedExitCriterias : public ExitCriterias
{
public:
bool syncSent;
bool syncRspReceived;
UninitializedExitCriterias() noexcept
: ExitCriterias(),
syncSent(false),
syncRspReceived(false) {}
UninitializedExitCriterias(const UninitializedExitCriterias&) = delete;
bool isFullfilled() const override
{
return ioResourceError || close || (syncSent && syncRspReceived);
}
void reset() override
{
ExitCriterias::reset();
syncSent = false;
syncRspReceived = false;
}
std::string toString() override
{
std::stringstream info;
info << "state:UNINITIALIZED " << ExitCriterias::toString()
<< " syncSent:" << syncSent
<< " syncRspReceived:" << syncRspReceived