Addressed Vincent's comments

Author: Donatien Garnier

features/FEATURE_BLE/ble/SecurityManager.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef _SECURITY_MANAGER_H_
-#define _SECURITY_MANAGER_H_
+#ifndef SECURITY_MANAGER_H_
+#define SECURITY_MANAGER_H_
 
 #include <stdint.h>
 
@@ -57,7 +57,7 @@
  * it. If either side doesn't support it Legacy Pairing will be used. This is an older standard of pairing.
  * If higher security is required legacy pairing can be disabled by calling allowLegacyPairing(false);
  *
- * How to use
+ * \par How to use
  *
  * First thing you need to do is to initialise the manager by calling init() with your chosen settings.
  *
@@ -87,9 +87,11 @@
  * accetPairing() or cancelPairing(). The result will be communicated on both peers through an event calling
  * pairingResult() in the EventHandler.
  *
+ * \par Sequence diagrams
  *
  * Sequence diagram "Just Works" pairing
  *
+ * \verbatim
  *  /----------- Device 1 --------------\  *------ BLE link ------*  /-------------- Device 2 -------------\
  *
  * App  EventHandler              SecurityManager              SecurityManager           EventHandler     App
@@ -101,14 +103,16 @@
  *  |        |                          |<---[pairing complete]----->|                        |            |
  *  |<- pairingResult() <---------------|                            |----------------> pairingResult() -->|
  *  |        |                          |                            |                        |            |
- *
+ * \endverbatim
+ * 
  *  @note the requestPairing() call isn't required to trigger pairing. Pairing will also be triggered
  *  if you request encryption and authentication and no bonding information is available. The sequence will
  *  be the same save for the lack of explicit requestPairing() call.
  *
  *
  *  Sequence diagram Encryption request when bonding information is available
  *
+ * \verbatim
  *  /----------- Device 1 --------------\  *------ BLE link ------*  /-------------- Device 2 -------------\
  *
  * App  EventHandler              SecurityManager              SecurityManager           EventHandler     App
@@ -117,13 +121,15 @@
  *  |       |                           |<-[encryption established]->|                        |            |
  *  |<- linkEncryptionResult() <--------|                            |---------> linkEncryptionResult() -->|
  *  |       |                           |                            |                        |            |
- *
+ * \endverbatim
+ * 
  * @note if bonding information is not available, pairing will be triggered
  *
  *
  * Sequence diagram for Secure Connections passkey entry pairing with one device having a display only
  * and other a keyboard
  *
+ * \verbatim
  *  /----------- Device 1 (keyboard) ---\  *------ BLE link ------*  /-------------- Device 2 (display) ---\
  *
  * App  EventHandler              SecurityManager              SecurityManager           EventHandler     App
@@ -143,7 +149,8 @@
  *  |        |                          |<---[pairing complete]----->|                        |            |
  *  |<- pairingResult() <---------------|                            |----------------> pairingResult() -->|
  *  |        |                          |                            |                        |            |
- *
+ * \endverbatim
+ * 
  */
 
 class SecurityManager {
@@ -267,9 +274,7 @@ class SecurityManager {
          * @param[in] whitelist pointer to the whitelist filled with entries based on bonding information
          */
         virtual void whitelistFromBondTable(Gap::Whitelist_t* whitelist) {
-            if (whitelist) {
-                delete whitelist;
-            }
+            (void)whitelist;
         }
 
         ////////////////////////////////////////////////////////////////////////////
@@ -1070,4 +1075,4 @@ class SecurityManager {
     LegacyEventHandler defaultEventHandler;
 };
 
-#endif /*_SECURITY_MANAGER_H_*/
+#endif /*SECURITY_MANAGER_H_*/

features/FEATURE_BLE/ble/generic/GenericGap.h

@@ -276,7 +276,7 @@ class GenericGap : public ::Gap,
     /** @note Implements ConnectionEventMonitor.
      *  @copydoc ConnectionEventMonitor::set_connection_event_handler
      */
-    void set_connection_event_handler(pal::ConnectionEventHandler *_connection_event_handler);
+    void set_connection_event_handler(pal::ConnectionEventMonitor::EventHandler *_connection_event_handler);
 
     void on_scan_timeout();
 
@@ -317,7 +317,7 @@ class GenericGap : public ::Gap,
     mutable Whitelist_t _whitelist;
     mbed::Timeout _advertising_timeout;
     mbed::Timeout _scan_timeout;
-    pal::ConnectionEventHandler *_connection_event_handler;
+    pal::ConnectionEventMonitor::EventHandler *_connection_event_handler;
 };
 
 }

features/FEATURE_BLE/ble/generic/GenericSecurityManager.h

@@ -28,16 +28,11 @@
 namespace ble {
 namespace generic {
 
-using pal::advertising_peer_address_type_t;
-using pal::AuthenticationMask;
-using pal::KeyDistribution;
-using pal::connection_peer_address_type_t;
-
 typedef SecurityManager::SecurityIOCapabilities_t SecurityIOCapabilities_t;
 
 class GenericSecurityManager : public SecurityManager,
                                public pal::SecurityManager::EventHandler,
-                               public pal::ConnectionEventHandler {
+                               public pal::ConnectionEventMonitor::EventHandler {
 public:
     typedef ble::pal::SecurityDistributionFlags_t SecurityDistributionFlags_t;
     typedef ble::pal::SecurityEntryKeys_t SecurityEntryKeys_t;
@@ -240,7 +235,7 @@ class GenericSecurityManager : public SecurityManager,
         _db(dbImpl),
         _connection_monitor(connMonitorImpl),
         _default_authentication(0),
-        _default_key_distribution(KeyDistribution::KEY_DISTRIBUTION_ALL),
+        _default_key_distribution(pal::KeyDistribution::KEY_DISTRIBUTION_ALL),
         _pairing_authorisation_required(false),
         _legacy_pairing_allowed(true),
         _master_sends_keys(false),
@@ -432,16 +427,16 @@ class GenericSecurityManager : public SecurityManager,
     struct ControlBlock_t : public pal::SecurityDistributionFlags_t {
         ControlBlock_t();
 
-        KeyDistribution get_initiator_key_distribution() {
-            return KeyDistribution(initiator_key_distribution);
+        pal::KeyDistribution get_initiator_key_distribution() {
+            return pal::KeyDistribution(initiator_key_distribution);
         };
-        KeyDistribution get_responder_key_distribution() {
-            return KeyDistribution(responder_key_distribution);
+        pal::KeyDistribution get_responder_key_distribution() {
+            return pal::KeyDistribution(responder_key_distribution);
         };
-        void set_initiator_key_distribution(KeyDistribution mask) {
+        void set_initiator_key_distribution(pal::KeyDistribution mask) {
             initiator_key_distribution = mask.value();
         };
-        void set_responder_key_distribution(KeyDistribution mask) {
+        void set_responder_key_distribution(pal::KeyDistribution mask) {
             responder_key_distribution = mask.value();
         };
 
@@ -476,8 +471,8 @@ class GenericSecurityManager : public SecurityManager,
     pal::SecurityDb &_db;
     pal::ConnectionEventMonitor &_connection_monitor;
 
-    AuthenticationMask _default_authentication;
-    KeyDistribution _default_key_distribution;
+    pal::AuthenticationMask _default_authentication;
+    pal::KeyDistribution _default_key_distribution;
 
     bool _pairing_authorisation_required;
     bool _legacy_pairing_allowed;
@@ -504,9 +499,9 @@ class GenericSecurityManager : public SecurityManager,
     virtual void on_pairing_request(
         connection_handle_t connection,
         bool use_oob,
-        AuthenticationMask authentication,
-        KeyDistribution initiator_dist,
-        KeyDistribution responder_dist
+        pal::AuthenticationMask authentication,
+        pal::KeyDistribution initiator_dist,
+        pal::KeyDistribution responder_dist
     );
 
     /** @copydoc ble::pal::SecurityManager::on_pairing_error
@@ -542,7 +537,7 @@ class GenericSecurityManager : public SecurityManager,
      */
     virtual void on_slave_security_request(
         connection_handle_t connection,
-        AuthenticationMask authentication
+        pal::AuthenticationMask authentication
     );
 
     ////////////////////////////////////////////////////////////////////////////
@@ -665,7 +660,7 @@ class GenericSecurityManager : public SecurityManager,
      */
     virtual void on_keys_distributed_bdaddr(
         connection_handle_t connection,
-        advertising_peer_address_type_t peer_address_type,
+        pal::advertising_peer_address_type_t peer_address_type,
         const address_t &peer_identity_address
     );
 

features/FEATURE_BLE/ble/pal/ConnectionEventMonitor.h

@@ -25,58 +25,58 @@ namespace ble {
 namespace pal {
 
 /**
- * Implemented by classes that are reacting to connection changes.
- * @see ConnectionEventMonitor
+ * Implemented by classes that need to be notified of connection changes.
+ * Notification is done by calling functions in the passed in event handler
  */
-class ConnectionEventHandler {
+class ConnectionEventMonitor {
 public:
     /**
-     * Inform the Security manager of a new connection. This will create
-     * or retrieve an existing security manager entry for the connected device.
-     * Called by GAP.
-     *
-     * @param[in] connection Handle to identify the connection.
-     * @param[in] role indicate if the device is central or peripheral.
-     * @param[in] peer_address_type type of address.
-     * @param[in] peer_address Address of the connected device.
-     * @param[in] local_address_type type of address of the local device.
-     * @param[in] local_address Address of the local device that was used during connection.
-     * @param[in] connection_params connection parameters like interval, latency and timeout.
+     * Implemented by classes that are reacting to connection changes.
+     * @see ConnectionEventMonitor
      */
-    virtual void on_connected(
-        connection_handle_t connection,
-        ::Gap::Role_t role,
-        BLEProtocol::AddressType_t peer_address_type,
-        const BLEProtocol::AddressBytes_t peer_address,
-        BLEProtocol::AddressType_t local_address_type,
-        const BLEProtocol::AddressBytes_t local_address,
-        const ::Gap::ConnectionParams_t *connection_params
-    ) = 0;
+    class EventHandler {
+    public:
+        /**
+         * Inform the Security manager of a new connection. This will create
+         * or retrieve an existing security manager entry for the connected device.
+         * Called by GAP.
+         *
+         * @param[in] connection Handle to identify the connection.
+         * @param[in] role indicate if the device is central or peripheral.
+         * @param[in] peer_address_type type of address.
+         * @param[in] peer_address Address of the connected device.
+         * @param[in] local_address_type type of address of the local device.
+         * @param[in] local_address Address of the local device that was used during connection.
+         * @param[in] connection_params connection parameters like interval, latency and timeout.
+         */
+        virtual void on_connected(
+            connection_handle_t connection,
+            ::Gap::Role_t role,
+            BLEProtocol::AddressType_t peer_address_type,
+            const BLEProtocol::AddressBytes_t peer_address,
+            BLEProtocol::AddressType_t local_address_type,
+            const BLEProtocol::AddressBytes_t local_address,
+            const ::Gap::ConnectionParams_t *connection_params
+        ) = 0;
 
-    /**
-     * Inform the monitor about a disconnection.
-     *
-     * @param[in] connectionHandle Handle to identify the connection.
-     * @param[in] reason Reason for the disconnection.
-     */
-    virtual void on_disconnected(
-        connection_handle_t connection,
-        ::Gap::DisconnectionReason_t reason
-    ) = 0;
-};
+        /**
+         * Inform the monitor about a disconnection.
+         *
+         * @param[in] connectionHandle Handle to identify the connection.
+         * @param[in] reason Reason for the disconnection.
+         */
+        virtual void on_disconnected(
+            connection_handle_t connection,
+            ::Gap::DisconnectionReason_t reason
+        ) = 0;
+    };
 
-/**
- * Implemented by classes that need to be notified of connection changes.
- * Notification is done by calling functions in the passed in event handler
- */
-class ConnectionEventMonitor {
-public:
     /**
      * Register a handler for connection events to be used internally and serviced first.
      *
      * @param[in] connection_event_handler Event handler being registered.
      */
-    virtual void set_connection_event_handler(ConnectionEventHandler *connection_event_handler) = 0;
+    virtual void set_connection_event_handler(EventHandler *connection_event_handler) = 0;
 };
 
 } // namespace pal

features/FEATURE_BLE/ble/pal/MemorySecurityDB.h

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-#ifndef _PAL_MEMORY_SECURITY_DB_H_
-#define _PAL_MEMORY_SECURITY_DB_H_
+#ifndef PAL_MEMORY_SECURITY_DB_H_
+#define PAL_MEMORY_SECURITY_DB_H_
 
 #include "SecurityDB.h"
 
@@ -369,4 +369,4 @@ class MemorySecurityDb : public SecurityDb {
 } /* namespace pal */
 } /* namespace ble */
 
-#endif /*_PAL_MEMORY_SECURITY_DB_H_*/
+#endif /*PAL_MEMORY_SECURITY_DB_H_*/

features/FEATURE_BLE/source/generic/GenericGap.cpp

@@ -1169,7 +1169,7 @@ bool GenericGap::initialize_whitelist() const
     return true;
 }
 
-void GenericGap::set_connection_event_handler(pal::ConnectionEventHandler *connection_event_handler)
+void GenericGap::set_connection_event_handler(pal::ConnectionEventMonitor::EventHandler *connection_event_handler)
 {
     _connection_event_handler = connection_event_handler;
 }

features/FEATURE_BLE/source/generic/GenericSecurityManager.cpp

@@ -21,6 +21,11 @@
 #include "mbedtls/cmac.h"
 #endif
 
+using ble::pal::advertising_peer_address_type_t;
+using ble::pal::AuthenticationMask;
+using ble::pal::KeyDistribution;
+using ble::pal::connection_peer_address_type_t;
+
 namespace ble {
 namespace generic {