Completely remove greybus operation

Now just using a simple message handler.

Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>

src/greybus_protocol.h

@@ -12,20 +12,12 @@
 #define GB_SVC_VERSION_MAJOR 0x00
 #define GB_SVC_VERSION_MINOR 0x01
 
-#define GB_COMMON_TYPE_PROTOCOL_VERSION 0x01
-
-/* Greybus Control request types */
-#define GB_CONTROL_TYPE_CPORT_SHUTDOWN 0x00
-#define GB_CONTROL_TYPE_PROTOCOL_VERSION 0x01
-#define GB_CONTROL_TYPE_GET_MANIFEST_SIZE 0x03
-#define GB_CONTROL_TYPE_GET_MANIFEST 0x04
-#define GB_CONTROL_TYPE_CONNECTED 0x05
-#define GB_CONTROL_TYPE_DISCONNECTED 0x06
-#define GB_CONTROL_TYPE_TIMESYNC_ENABLE 0x07
 
 /* Greybus SVC request types */
-#define GB_SVC_TYPE_PING 0x13
+#define GB_SVC_TYPE_PROTOCOL_VERSION_REQUEST 0x01
 #define GB_SVC_TYPE_PROTOCOL_VERSION_RESPONSE 0x81
+#define GB_SVC_TYPE_PING_REQUEST 0x13
+#define GB_SVC_TYPE_PING_RESPONSE 0x93
 
 /*
  * All operation messages (both requests and responses) begin with

src/main.c

@@ -51,14 +51,10 @@ static void apbridge_entry(void *p1, void *p2, void *p3) {
   while (1) {
     // Go through all connections
     SYS_DLIST_FOR_EACH_CONTAINER(&gb_connections_list, conn, node) {
-      LOG_DBG("Start Reading from AP");
       msg = conn->inf_ap->controller.read(&conn->inf_ap->controller, conn->ap_cport_id);
       if (msg != NULL)  {
-        LOG_DBG("Start Writing by Peer");
         conn->inf_peer->controller.write(&conn->inf_peer->controller, msg, conn->peer_cport_id);
-        LOG_DBG("Successful Writing by Peer");
       }
-      LOG_DBG("Start Reading from Peer");
       msg = conn->inf_peer->controller.read(&conn->inf_peer->controller, conn->peer_cport_id);
       if(msg != NULL) {
         conn->inf_ap->controller.write(&conn->inf_ap->controller, msg, conn->ap_cport_id);

src/operations.c

@@ -12,138 +12,14 @@
 
 LOG_MODULE_DECLARE(cc1352_greybus, CONFIG_BEAGLEPLAY_GREYBUS_LOG_LEVEL);
 
-K_MUTEX_DEFINE(gb_operations_mutex);
-K_MUTEX_DEFINE(gb_operations_callback_mutex);
-
-static void callback_work_handler(struct k_work *);
-
 static atomic_t operation_id_counter = ATOMIC_INIT(1);
-static sys_dlist_t greybus_operations_list =
-    SYS_DLIST_STATIC_INIT(&greybus_operations_list);
-static sys_dlist_t greybus_operations_callback_list =
-    SYS_DLIST_STATIC_INIT(&greybus_operations_callback_list);
-
-K_WORK_DEFINE(callback_work, callback_work_handler);
-
-static void gb_operation_dealloc(struct gb_operation *op) {
-  if (op == NULL) {
-    return;
-  }
-
-  LOG_DBG("Dealloc Request");
-  gb_message_dealloc(op->request);
-  LOG_DBG("Dealloc Response");
-  gb_message_dealloc(op->response);
-
-  LOG_DBG("Free Operation");
-  k_free(op);
-}
-
-static void callback_work_handler(struct k_work *work) {
-  struct gb_operation *op;
-  sys_dnode_t *head_node;
-
-  while (1) {
-    k_mutex_lock(&gb_operations_callback_mutex, K_FOREVER);
-    head_node = sys_dlist_get(&greybus_operations_callback_list);
-    k_mutex_unlock(&gb_operations_callback_mutex);
-
-    if (head_node == NULL) {
-      return;
-    }
-
-    op = SYS_DLIST_CONTAINER(head_node, op, node);
-
-    if (op->callback != NULL) {
-      op->callback(op);
-    }
-
-    LOG_DBG("Dealloc Operation %d", op->operation_id);
-    gb_operation_dealloc(op);
-    LOG_DBG("Finish Dealloc Operation %d", op->operation_id);
-  }
-}
-
-static void gb_operation_finish(struct gb_operation *op) {
-  sys_dlist_remove(&op->node);
-
-  k_mutex_lock(&gb_operations_callback_mutex, K_FOREVER);
-  sys_dlist_append(&greybus_operations_callback_list, &op->node);
-  k_mutex_unlock(&gb_operations_callback_mutex);
-
-  k_work_submit(&callback_work);
-}
-
-static struct gb_operation *gb_operation_find_by_id(uint16_t operation_id,
-                                                    sys_dlist_t *list) {
-  struct gb_operation *op;
-
-  SYS_DLIST_FOR_EACH_CONTAINER(list, op, node) {
-    if (op->operation_id == operation_id) {
-      return op;
-    }
-  }
-  return NULL;
-}
-
-struct gb_operation *gb_operation_alloc(bool is_oneshot) {
-  struct gb_operation *op = k_malloc(sizeof(struct gb_operation));
-  if (!op) {
-    LOG_ERR("Failed to allocate Greybus Operation");
-    return NULL;
-  }
-  op->response = NULL;
-  op->request = NULL;
-  op->request_sent = false;
-  op->response_received = false;
-  op->callback = NULL;
-
-  if (is_oneshot) {
-    op->operation_id = 0;
-  } else {
-    atomic_val_t temp = atomic_inc(&operation_id_counter);
-    if (temp == UINT16_MAX) {
-      atomic_set(&operation_id_counter, 1);
-    }
-    op->operation_id = temp;
-  }
-
-  return op;
-}
-
-void gb_operation_queue(struct gb_operation *op) {
-  k_mutex_lock(&gb_operations_mutex, K_FOREVER);
-  sys_dlist_append(&greybus_operations_list, &op->node);
-  k_mutex_unlock(&gb_operations_mutex);
-}
-
-int gb_operation_request_alloc(struct gb_operation *op, const void *payload,
-                               size_t payload_len, uint8_t request_type,
-                               greybus_operation_callback_t callback) {
-  int ret;
 
-  op->request = k_malloc(sizeof(struct gb_message) + payload_len);
-  if (op->request == NULL) {
-    LOG_WRN("Failed to allocate Greybus request message");
-    ret = -E_NO_HEAP_MEM;
-    goto early_exit;
+static uint16_t new_operation_id() {
+  atomic_val_t temp = atomic_inc(&operation_id_counter);
+  if (temp == UINT16_MAX) {
+    atomic_set(&operation_id_counter, 1);
   }
-
-  op->request->header.size = sizeof(struct gb_operation_msg_hdr) + payload_len;
-  op->request->header.id = op->operation_id;
-  op->request->header.type = request_type;
-  op->request->header.status = 0;
-
-  memcpy(op->request->payload, payload, payload_len);
-  op->request->payload_size = payload_len;
-
-  op->request->operation = op;
-  op->callback = callback;
-
-  ret = SUCCESS;
-
-early_exit:
-  return ret;
+  return temp;
 }
 
 void gb_message_dealloc(struct gb_message *msg) {
@@ -151,29 +27,9 @@ void gb_message_dealloc(struct gb_message *msg) {
     return;
   }
 
-  LOG_DBG("Free Message");
   k_free(msg);
 }
 
-int gb_operation_set_response(struct gb_message *msg) {
-  struct gb_operation *op;
-
-  k_mutex_lock(&gb_operations_mutex, K_FOREVER);
-  op = gb_operation_find_by_id(msg->header.id, &greybus_operations_list);
-  if (op == NULL || op->response_received) {
-    return -E_CLIENT_REQUEST;
-  }
-
-  msg->operation = op;
-  op->response = msg;
-  op->response_received = true;
-  LOG_DBG("Operation with ID %u completed", msg->header.id);
-  gb_operation_finish(op);
-  k_mutex_unlock(&gb_operations_mutex);
-
-  return SUCCESS;
-}
-
 int gb_message_hdlc_send(const struct gb_message *msg) {
   char buffer[50];
 
@@ -204,17 +60,10 @@ struct gb_connection *gb_create_connection(struct gb_interface *inf_ap,
   return conn;
 }
 
-static uint16_t new_operation_id() {
-  atomic_val_t temp = atomic_inc(&operation_id_counter);
-  if (temp == UINT16_MAX) {
-    atomic_set(&operation_id_counter, 1);
-  }
-  return temp;
-}
-
 struct gb_message *gb_message_request_alloc(const void *payload,
                                             size_t payload_len,
-                                            uint8_t request_type, bool is_oneshot) {
+                                            uint8_t request_type,
+                                            bool is_oneshot) {
   struct gb_message *msg;
 
   msg = k_malloc(sizeof(struct gb_message) + payload_len);

src/operations.h

@@ -17,48 +17,21 @@
 #define E_NULL_REQUEST 1
 #define E_ALREADY_SENT 2
 
-struct gb_operation;
-struct gb_message;
-
-typedef void (*greybus_operation_callback_t)(struct gb_operation *);
-
 /*
- * Struct to represent greybus message
+ * Struct to represent greybus message. This is a variable sized type.
  *
- * @param operation: greybus operation this message is associated with. Can be
- * NULL in case of message received.
+ * @param fifo_reserved: reserved for fifo
  * @param header: greybus msg header.
- * @param payload: heap allocated payload.
  * @param payload_size: size of payload in bytes
+ * @param payload: heap allocated payload.
  */
 struct gb_message {
   void *fifo_reserved;
-  struct gb_operation *operation;
   struct gb_operation_msg_hdr header;
   size_t payload_size;
   uint8_t payload[];
 };
 
-/*
- * Struct to represent a greybus operation.
- *
- * @param operation_id: the unique id for this operation.
- * @param request_sent: flag to check if the request has been sent.
- * @param request: pointer to greybus request message.
- * @param response: pointer to greybus response message.
- * @param callback: callback function called when operation is completed.
- * @param node: operation dlist node.
- */
-struct gb_operation {
-  uint16_t operation_id;
-  bool request_sent;
-  bool response_received;
-  struct gb_message *request;
-  struct gb_message *response;
-  greybus_operation_callback_t callback;
-  sys_dnode_t node;
-};
-
 /*
  * Controller for each greybus interface
  *
@@ -73,11 +46,25 @@ struct gb_controller {
   void *ctrl_data;
 };
 
+/*
+ * A greybus interface. Can have multiple Cports
+ *
+ * @param id: Interface ID
+ * @param controller: A controller which provides operations for this interface
+ */
 struct gb_interface {
   uint8_t id;
   struct gb_controller controller;
 };
 
+/*
+ * A connection between two greybus interfaces
+ *
+ * @param inf_ap: Greybus interface of AP.
+ * @param inf_peer: Greybus interface of the peer
+ * @param ap_cport_id: Cport of AP to connect to.
+ * @param peer_cport_id: Cport of Peer to connect to.
+ */
 struct gb_connection {
   struct gb_interface *inf_ap;
   struct gb_interface *inf_peer;
@@ -87,17 +74,14 @@ struct gb_connection {
 };
 
 /*
- * Check if the greybus operation is unidirectional.
- * These messages will not have a response and thus should be freed once request
- * is sent.
+ * Check if the greybus message header is a response.
  *
- * @param op: greybus operation
+ * @param hdr: greybus header
  *
- * @return true if operation is unidirectional, else false.
+ * @return true if message is response, else false.
  */
-static inline bool
-gb_operation_is_unidirectional(const struct gb_operation *op) {
-  return op->operation_id == 0;
+static inline bool gb_hdr_is_response(const struct gb_operation_msg_hdr *hdr) {
+  return hdr->type & GB_TYPE_RESPONSE_FLAG;
 }
 
 /*
@@ -108,80 +92,48 @@ gb_operation_is_unidirectional(const struct gb_operation *op) {
  * @return true if message is response, else false.
  */
 static inline bool gb_message_is_response(const struct gb_message *msg) {
-  return msg->header.type & GB_TYPE_RESPONSE_FLAG;
-}
-
-static inline bool gb_hdr_is_response(const struct gb_operation_msg_hdr *hdr) {
-  return hdr->type & GB_TYPE_RESPONSE_FLAG;
+  return gb_hdr_is_response(&msg->header);
 }
 
 /*
- * Check if the request for the operation has been sent.
- *
- * @param op: greybus operation
- *
- * @return true if already sent, else false.
- */
-static inline bool gb_operation_request_sent(const struct gb_operation *op) {
-  return op->request_sent;
-}
-
-/*
- * Allocate a greybus operation.
- *
- * Note: This does not allocate a request or a response.
- *
- * @param is_oneshot: flag to indicate if the request is unidirectional.
+ * Deallocate a greybus message.
  *
- * @return heap allocated gb_operation
+ * @param pointer to the message to deallcate
  */
-struct gb_operation *gb_operation_alloc(bool);
+void gb_message_dealloc(struct gb_message *);
 
 /*
- * Allocate a request on a pre-allocated greybus operation
+ * Send a greybus message over HDLC
  *
- * @param op: pointer to greybus operation
- * @param data: pointer to payload for request
- * @param payload_len: size of payload
- * @param type: type of greybus operation
- * @param callback: greybus callback to call on operation completion.
- *
- * @return 0 on success, negative in case of error
+ * @param Greybus message
  */
-int gb_operation_request_alloc(struct gb_operation *, const void *, size_t,
-                               uint8_t, greybus_operation_callback_t);
+int gb_message_hdlc_send(const struct gb_message *);
 
 /*
- * Queue an operation to be executed.
- *
- * Note: This is asynchronous.
+ * Create a greybus connection between two interfaces
  *
- * @param op: greybus operation to execute
- */
-void gb_operation_queue(struct gb_operation *);
-
-/*
- * Deallocate a greybus message.
+ * @param Greybus AP Interface
+ * @param Greybus Peer Interface
+ * @param Greybus AP Interface Cport ID
+ * @param Greybus Peer Interface Cport ID
  *
- * @param pointer to the message to deallcate
+ * @return greybus connection allocated on heap. Null in case of errro
  */
-void gb_message_dealloc(struct gb_message *);
+struct gb_connection *gb_create_connection(struct gb_interface *,
+                                           struct gb_interface *, uint16_t,
+                                           uint16_t);
 
 /*
- * Check if a received greybus message is a response to an operation.
+ * Allocate a greybus request message
  *
- * @param received message
+ * @param Payload
+ * @param Payload len
+ * @param Request Type
+ * @param Is one shot
  *
- * @return 0 in case of success. negative in case of error
+ * @return greybus message allocated on heap. Null in case of errro
  */
-int gb_operation_set_response(struct gb_message *);
-
-int gb_message_hdlc_send(const struct gb_message *);
-
-struct gb_connection *gb_create_connection(struct gb_interface *,
-                                           struct gb_interface *, uint16_t,
-                                           uint16_t);
-
-struct gb_message *gb_message_request_alloc(const void *, size_t, uint8_t, bool);
+struct gb_message *gb_message_request_alloc(const void *, size_t, uint8_t,
+                                            bool);
 
 #endif

src/svc.c

@@ -13,16 +13,24 @@ struct gb_common_version_request {
   uint8_t minor;
 } __packed;
 
-static void gb_common_version_callback(struct gb_message *msg) {
+static void svc_version_response_handler(struct gb_message *msg) {
   struct gb_common_version_request *response =
       (struct gb_common_version_request *)msg->payload;
   LOG_DBG("SVC Protocol Version %u.%u", response->major, response->minor);
 }
 
+static void svc_ping_response_handler(struct gb_message *msg) {
+  ARG_UNUSED(msg);
+  LOG_DBG("Received Pong");
+}
+
 static void gb_handle_msg(struct gb_message *msg) {
   switch (msg->header.type) {
   case GB_SVC_TYPE_PROTOCOL_VERSION_RESPONSE:
-    gb_common_version_callback(msg);
+    svc_version_response_handler(msg);
+    break;
+  case GB_SVC_TYPE_PING_RESPONSE:
+    svc_ping_response_handler(msg);
     break;
   default:
     LOG_WRN("Handling SVC operation Type %u not supported yet",
@@ -55,10 +63,6 @@ static struct gb_interface intf = {.id = SVC_INF_ID,
                                                   .write = svc_inf_write,
                                                   .ctrl_data = &svc_ctrl_data}};
 
-static void svc_ping_callback(struct gb_operation *op) {
-  LOG_DBG("Received Pong");
-}
-
 static int control_send_request(void *payload, size_t payload_len,
                                 uint8_t request_type) {
   struct gb_message *msg;
@@ -76,10 +80,12 @@ int svc_send_version() {
   struct gb_common_version_request req = {.major = GB_SVC_VERSION_MAJOR,
                                           .minor = GB_SVC_VERSION_MINOR};
   return control_send_request(&req, sizeof(struct gb_common_version_request),
-                              GB_COMMON_TYPE_PROTOCOL_VERSION);
+                              GB_SVC_TYPE_PROTOCOL_VERSION_REQUEST);
 }
 
-int svc_send_ping() { return control_send_request(NULL, 0, GB_SVC_TYPE_PING); }
+int svc_send_ping() {
+  return control_send_request(NULL, 0, GB_SVC_TYPE_PING_REQUEST);
+}
 
 struct gb_interface *svc_init() {
   k_fifo_init(&svc_ctrl_data.pending_read);

src/svc.h

@@ -6,18 +6,25 @@
 #define SVC_INF_ID 0
 
 /*
- * Create CONTROL_TYPE_PING greybus operation and queue it for sending.
+ * Create CONTROL_TYPE_PING greybus message and queue it for sending.
  *
  * Note: This does not immediately send the request.
  *
- * @param sock: Socket for the cport
- *
  * @return 0 if successful, else error.
  */
 int svc_send_ping();
 
+/*
+ * Create SVC_TYPE_VERSION greybus message and queue it for sending.
+ *
+ * @return 0 if successful, else error.
+ */
 int svc_send_version();
 
+/*
+ * Initialize SVC Interface. Should be called before sending any greybus
+ * request.
+ */
 struct gb_interface *svc_init();
 
 #endif