chore: address review comments & cleanup work

packages/atclient/include/atclient/connection.h

@@ -1,7 +1,7 @@
 /*
  *
  * The connection family of types and methods represents a single connection to
- * if you want a pure socket representation see net_socket.h.
+ * if you want a pure socket representation see socket.h.
  *
  * At the moment _socket represents a singular tcp socket, but in the future it may be altered
  * to be a union of different connection types, such as a websocket or other construct.

packages/atclient/include/atclient/monitor.h

@@ -6,14 +6,10 @@ extern "C" {
 
 #include "atclient/atclient.h"
 #include "atclient/atnotification.h"
+#include "atclient/socket.h"
 #include <atchops/platform.h> // IWYU pragma: keep
 #include <stdbool.h>
 
-// HACK let's just get it working for now this is so wrong
-#ifndef MBEDTLS_ERR_SSL_TIMEOUT
-#define MBEDTLS_ERR_SSL_TIMEOUT -37
-#endif
-
 /**
  * @brief Represents a message received from the monitor connection, typically derived from the prefix of the response
  * (e.g. "data:ok"'s message type would be "data" = ATCLIENT_MONITOR_MESSAGE_TYPE_DATA_RESPONSE)

packages/atclient/include/atclient/socket.h

@@ -10,6 +10,22 @@
 extern "C" {
 #endif
 
+#ifndef ATCLIENT_SSL_TIMEOUT_EXITCODE
+
+#if defined(ATCLIENT_SOCKET_PROVIDER_MBEDTLS)
+#define ATCLIENT_SSL_TIMEOUT_EXITCODE MBEDTLS_ERR_SSL_TIMEOUT
+
+#elif defined(ATCLIENT_SOCKET_PROVIDER_ARDUINO_BEARSSL)
+// Most arduino libraries only use -1 or positive integers
+#define ATCLIENT_SSL_TIMEOUT_EXITCODE -101
+
+#else
+#error "ATCLIENT_ERR_SSL_TIMEOUT is undefined"
+
+#endif
+
+#endif
+
 // IWYU pragma: begin_exports
 
 // Export the appropriate platform specific struct implementation

packages/atclient/include/atclient/socket_mbedtls.h

@@ -1,7 +1,7 @@
-// IWYU pragma: private, include "atclient/net_socket.h"
-// IWYU pragma: friend "net_socket_mbedtls.*"
-#ifndef ATCLIENT_NET_SOCKET_MBEDTLS_H
-#define ATCLIENT_NET_SOCKET_MBEDTLS_H
+// IWYU pragma: private, include "atclient/socket.h"
+// IWYU pragma: friend "socket_mbedtls.*"
+#ifndef ATCLIENT_SOCKET_MBEDTLS_H
+#define ATCLIENT_SOCKET_MBEDTLS_H
 #include <atchops/platform.h>
 #if defined(ATCLIENT_SOCKET_PROVIDER_MBEDTLS)
 #include <atclient/socket_shared.h>
@@ -14,7 +14,7 @@
 extern "C" {
 #endif
 
-// Make this type more portable to consume later
+// TODO: Make this type more portable to consume later
 struct atclient_raw_socket {
   mbedtls_net_context net;
 };

packages/atclient/include/atclient/socket_shared.h

@@ -1,5 +1,5 @@
-// IWYU pragma: private, include "atclient/net_socket.h"
-// IWYU pragma: friend "net_socket_mbedtls.*"
+// IWYU pragma: private, include "atclient/socket.h"
+// IWYU pragma: friend "socket_mbedtls.*"
 #ifndef ATCLIENT_SOCKET_SHARED_H
 #define ATCLIENT_SOCKET_SHARED_H
 #include <stddef.h>

packages/atclient/src/monitor.c

@@ -113,18 +113,15 @@ int atclient_monitor_read(atclient *monitor_conn, atclient *atclient, atclient_m
   int ret = atclient_tls_socket_read(&monitor_conn->atserver_connection._socket, &buffer, &buffer_len,
                                      atclient_socket_read_until_char('@'));
 
-  // TODO: move this later for now it's fine as it should
-  if (ret == MBEDTLS_ERR_SSL_TIMEOUT) {
+  if (ret == ATCLIENT_SSL_TIMEOUT_EXITCODE) {
     // treat a timeout as empty message, non error
     message->type = ATCLIENT_MONITOR_MESSAGE_TYPE_EMPTY;
     ret = 0;
     goto exit;
-  }
-
-  if (ret <= 0) { // you should reconnect...
+  } else if (ret != 0) { // you should reconnect...
     message->type = ATCLIENT_MONITOR_ERROR_READ;
     message->error_read.error_code = ret;
-    atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "Read nothing from the monitor connection: %d\n", ret);
+    atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "Error: monitor exited with code %d\n", ret);
     goto exit;
   }
 

packages/atclient/src/socket.c

@@ -1,7 +1,5 @@
 #include <atchops/platform.h>
 #include <atclient/socket.h>
-// Most of the implementation for net_socket is platform specific,
-// See the other net_socket_*.c files
 
 struct atclient_socket_read_options atclient_socket_read_until_char(char until) {
   return (struct atclient_socket_read_options){

packages/atclient/src/socket_mbedtls.c

@@ -1,6 +1,7 @@
 // These two headers must be included in a specific order
 #include "atchops/platform.h" // IWYU pragma: keep
 // Don't move them
+#include "atclient/monitor.h"
 #include "atclient/socket.h"
 
 #if defined(ATCLIENT_SOCKET_PROVIDER_MBEDTLS)
@@ -273,96 +274,6 @@ int atclient_tls_socket_read(struct atclient_tls_socket *socket, unsigned char *
     return 4;
   }
 }
-// int atclient_tls_socket_read_num_bytes(struct atclient_tls_socket *socket, unsigned char **value, size_t *value_len,
-//                                        size_t num_bytes) {
-//   // Assume params have been validated by socket_read
-//   int ret;
-//   unsigned char *recv = NULL;
-//   size_t blocks = 0; // number of allocated blocks
-//
-//   do {
-//     size_t offset = READ_BLOCK_LEN * blocks; // offset to current block
-//     // Allocate memory
-//     unsigned char *temp = realloc(recv, sizeof(unsigned char) * (offset + READ_BLOCK_LEN));
-//     if (temp == NULL) {
-//       atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "Failed to allocate receive buffer\n");
-//       if (recv != NULL) {
-//         free(recv);
-//       }
-//       return 1;
-//     }
-//     recv = temp; // once we ensure realloc was successful we set recv to the new memory
-//
-//     // Read into current block
-//     size_t pos = 0; // position in current block
-//     do {
-//       size_t remaining_for_block;
-//       if (READ_BLOCK_LEN + offset > num_bytes) {
-//         // We are in the final block, so only read the amount that will make
-//         // us reach num_bytes
-//         remaining_for_block = num_bytes - (offset + pos);
-//       } else {
-//         remaining_for_block = READ_BLOCK_LEN - pos;
-//       }
-//
-//       ret = mbedtls_ssl_read(&socket->ssl, recv + offset + pos, remaining_for_block);
-//       if (ret > 0) {
-//         pos += ret; // successful read, increment pos
-//
-//         if (offset + pos == num_bytes) {
-//           *value = recv;
-//           *value_len = (offset + pos);
-//           return 0; // The only return where recv should not be freed
-//         }
-//
-//         continue; // not done reading yet
-//       }
-//
-//       // handle non-happy path
-//       switch (ret) {
-//       case 0:                                 // connection is closed
-//       case MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY: // connection is closed
-//         atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "Socket closed while reading: %d\n", ret);
-//         ret = MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY; // ensure ret val is not 0
-//         free(recv);
-//         return ret;
-//       case MBEDTLS_ERR_SSL_WANT_READ:          // handshake incomplete
-//       case MBEDTLS_ERR_SSL_WANT_WRITE:         // handshake incomplete
-//       case MBEDTLS_ERR_SSL_ASYNC_IN_PROGRESS:  // async operation in progress
-//       case MBEDTLS_ERR_SSL_CRYPTO_IN_PROGRESS: // crypto operation in progress
-//                                                // async error, we need to try again
-//         break;
-//       case MBEDTLS_ERR_SSL_TIMEOUT: // treat a timeout as
-//
-//         if (value != NULL) {
-//           *value = recv;
-//         } else {
-//           free(recv);
-//         }
-//        if (value_len != NULL) {
-//           *value_len = (offset + pos);
-//         }
-//         return 0; // The only return where recv should not be freed
-//         // unexpected errors while reading
-//       default:
-//         if (ret > 0) {
-//           atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "Unexpected read value %d\n", ret);
-//         } else {
-//           char strerr[512];
-//           mbedtls_strerror(ret, strerr, 512);
-//           atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "%s", strerr);
-//         }
-//         free(recv);
-//         return ret;
-//       } // don't put anything after switch without checking it first
-//     } while (pos < READ_BLOCK_LEN);
-//     blocks++;
-//   } while (blocks < MAX_READ_BLOCKS);
-//   // We should only arrive at this point if we max out blocks
-//   // Every other code path should return early
-//   atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_ERROR, "Failed to read within the maximum allowed number of read
-//   blocks\n"); free(recv); return 1;
-// }
 
 int atclient_tls_socket_read_until_char(struct atclient_tls_socket *socket, unsigned char **value, size_t *value_len,
                                         char until_char) {
@@ -409,6 +320,7 @@ int atclient_tls_socket_read_until_char(struct atclient_tls_socket *socket, unsi
         continue;   // continue if not found char yet
       }
       // handle non-happy path
+      atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "Socket read error: %d", ret);
       switch (ret) {
       case 0:                                 // connection is closed
       case MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY: // connection is closed
@@ -428,7 +340,7 @@ int atclient_tls_socket_read_until_char(struct atclient_tls_socket *socket, unsi
         if (timeout_count == MAX_READ_TIMEOUTS) {
           atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "Failed to read the full message after %d attempts\n",
                        MAX_READ_TIMEOUTS);
-          return ret;
+          return ATCLIENT_SSL_TIMEOUT_EXITCODE;
         }
         usleep(1000);
         break;

tests/functional_tests/tests/test_atclient_get_atkeys.c

@@ -146,7 +146,6 @@ exit: {
 
 static int test_4_atclient_get_atkeys_null_regex(atclient *ctx, const char *scan_regex, const bool showhidden) {
   int ret = 1;
-  return 0; // NOTE: disabled temporarily DONT LET THIS GET MERGED
 
   atlogger_log(TAG, ATLOGGER_LOGGING_LEVEL_DEBUG, "test_4_atclient_get_atkeys_null_regex\n");