acassis commented on code in PR #3459:
URL: https://github.com/apache/nuttx-apps/pull/3459#discussion_r3102400880


##########
modbus/nxmodbus/core/nxmb_context.c:
##########
@@ -0,0 +1,392 @@
+/****************************************************************************
+ * apps/modbus/nxmodbus/core/nxmb_context.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <nuttx/compiler.h>
+#include <nuttx/debug.h>
+
+#include <errno.h>
+#include <pthread.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <nxmodbus/nxmodbus.h>
+
+#include "nxmb_internal.h"
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static struct nxmb_context_s g_nxmb_pool[CONFIG_NXMODBUS_MAX_INSTANCES];
+static pthread_mutex_t       g_pool_lock = PTHREAD_MUTEX_INITIALIZER;
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: nxmb_create
+ *
+ * Description:
+ *   Create a new NxModbus instance.
+ *
+ * Input Parameters:
+ *   handle - Pointer to receive the instance handle
+ *   config - Instance configuration
+ *
+ * Returned Value:
+ *   0 on success, negative errno on failure
+ *
+ ****************************************************************************/
+
+int nxmb_create(FAR nxmb_handle_t *handle,
+                FAR const struct nxmb_config_s *config)
+{
+  nxmb_handle_t ctx = NULL;
+  int           ret = -ENOMEM;
+  int           i;
+
+  DEBUGASSERT(handle && config);
+
+#ifndef CONFIG_NXMODBUS_CLIENT
+  if (config->is_client)
+    {
+      /* Master not supported */
+
+      return -EINVAL;
+    }
+#endif
+
+  pthread_mutex_lock(&g_pool_lock);
+
+  for (i = 0; i < CONFIG_NXMODBUS_MAX_INSTANCES; i++)
+    {
+      if (g_nxmb_pool[i].mode == NXMB_MODE_INVAL)
+        {
+          ctx = &g_nxmb_pool[i];
+          break;
+        }
+    }
+
+  if (ctx != NULL)
+    {
+      memset(ctx, 0, sizeof(struct nxmb_context_s));
+
+      ret = pthread_mutex_init(&ctx->lock, NULL);
+      if (ret == 0)
+        {
+          ctx->mode      = config->mode;
+          ctx->unit_id   = config->unit_id;
+          ctx->is_client = config->is_client;
+
+          switch (config->mode)
+            {
+#ifdef CONFIG_NXMODBUS_RTU
+              case NXMB_MODE_RTU:
+                ctx->transport_cfg.serial = config->transport.serial;
+                ctx->transport_ops        = &g_nxmb_serial_ops;
+                break;
+#endif
+
+#ifdef CONFIG_NXMODBUS_ASCII
+              case NXMB_MODE_ASCII:
+                ctx->transport_cfg.serial = config->transport.serial;
+                ctx->transport_ops        = &g_nxmb_ascii_ops;
+                break;
+#endif
+
+#ifdef CONFIG_NXMODBUS_TCP
+              case NXMB_MODE_TCP:
+                ctx->transport_cfg.tcp = config->transport.tcp;
+                ctx->transport_ops     = &g_nxmb_tcp_ops;
+                break;
+#endif
+
+#ifdef CONFIG_NXMODBUS_RAW_ADU
+              case NXMB_MODE_RAW:
+                ctx->transport_cfg.raw = config->transport.raw;
+                ctx->transport_ops     = &g_nxmb_raw_ops;
+                break;
+#endif
+
+              default:
+                pthread_mutex_destroy(&ctx->lock);
+                memset(ctx, 0, sizeof(struct nxmb_context_s));
+                ret = -EINVAL;
+                goto out;
+            }
+
+          *handle = ctx;
+          ret     = 0;
+        }
+      else
+        {
+          memset(ctx, 0, sizeof(struct nxmb_context_s));
+          ret = -ret;
+        }
+    }
+
+out:
+  pthread_mutex_unlock(&g_pool_lock);
+  return ret;
+}
+
+/****************************************************************************
+ * Name: nxmb_destroy
+ *
+ * Description:
+ *   Destroy an NxModbus instance.
+ *
+ * Input Parameters:
+ *   handle - Instance handle
+ *
+ * Returned Value:
+ *   0 on success, negative errno on failure
+ *
+ ****************************************************************************/
+
+int nxmb_destroy(nxmb_handle_t ctx)
+{
+#ifdef CONFIG_NXMODBUS_CUSTOM_FC
+  FAR struct nxmb_custom_fc_s *entry;
+  FAR struct nxmb_custom_fc_s *next;
+#endif
+
+  DEBUGASSERT(ctx);
+
+  pthread_mutex_lock(&g_pool_lock);
+  pthread_mutex_lock(&ctx->lock);
+
+  if (ctx->enabled)
+    {
+      pthread_mutex_unlock(&ctx->lock);
+      pthread_mutex_unlock(&g_pool_lock);
+      return -EBUSY;
+    }
+
+  pthread_mutex_unlock(&ctx->lock);
+  pthread_mutex_destroy(&ctx->lock);
+
+#ifdef CONFIG_NXMODBUS_CUSTOM_FC
+  entry = ctx->custom_fc_list;
+  while (entry != NULL)
+    {
+      next = entry->next;
+      free(entry);
+      entry = next;
+    }
+#endif
+
+  memset(ctx, 0, sizeof(struct nxmb_context_s));
+  pthread_mutex_unlock(&g_pool_lock);
+
+  return 0;
+}
+
+/****************************************************************************
+ * Name: nxmb_set_callbacks
+ *
+ * Description:
+ *   Set application callbacks for register access.
+ *
+ * Input Parameters:
+ *   handle    - Instance handle
+ *   callbacks - Pointer to callback structure
+ *
+ * Returned Value:
+ *   0 on success, negative errno on failure
+ *
+ ****************************************************************************/
+
+int nxmb_set_callbacks(nxmb_handle_t ctx,
+                       FAR const struct nxmb_callbacks_s *callbacks)
+{
+  DEBUGASSERT(ctx);
+
+  pthread_mutex_lock(&ctx->lock);
+  ctx->callbacks = callbacks;
+  pthread_mutex_unlock(&ctx->lock);
+
+  return 0;
+}
+
+/****************************************************************************
+ * Name: nxmb_enable
+ *
+ * Description:
+ *   Enable an NxModbus instance and initialize transport.
+ *
+ * Input Parameters:
+ *   handle - Instance handle
+ *
+ * Returned Value:
+ *   0 on success, negative errno on failure
+ *
+ ****************************************************************************/
+
+int nxmb_enable(nxmb_handle_t ctx)
+{
+  int ret;
+
+  DEBUGASSERT(ctx && ctx->transport_ops && ctx->transport_ops->init);
+
+  pthread_mutex_lock(&ctx->lock);
+
+  if (ctx->enabled)
+    {
+      pthread_mutex_unlock(&ctx->lock);
+      return -EBUSY;
+    }
+
+#ifdef CONFIG_NXMODBUS_CLIENT
+  if (ctx->is_client)
+    {
+      ret = nxmb_client_init(ctx);
+      if (ret < 0)
+        {
+          pthread_mutex_unlock(&ctx->lock);
+          return ret;
+        }
+    }
+#endif
+
+  /* Initialize transport */
+
+  ret = ctx->transport_ops->init(ctx);
+  if (ret < 0)
+    {
+#ifdef CONFIG_NXMODBUS_CLIENT
+      if (ctx->is_client)
+        {
+          nxmb_client_deinit(ctx);
+        }
+#endif
+
+      pthread_mutex_unlock(&ctx->lock);
+      return ret;
+    }
+
+  ctx->enabled = true;
+  pthread_mutex_unlock(&ctx->lock);
+
+  return 0;
+}
+
+/****************************************************************************
+ * Name: nxmb_set_server_id
+ *
+ * Description:
+ *   Configure the data returned by FC17 (Report Server ID).
+ *
+ * Input Parameters:
+ *   handle     - Instance handle
+ *   id         - Server ID byte
+ *   is_running - True if device is running
+ *   additional - Optional additional data (may be NULL)
+ *   addlen     - Length of additional data
+ *
+ * Returned Value:
+ *   0 on success, negative errno on failure
+ *
+ ****************************************************************************/
+
+int nxmb_set_server_id(nxmb_handle_t ctx, uint8_t id, bool is_running,
+                       FAR const uint8_t *additional, uint16_t addlen)
+{
+  DEBUGASSERT(ctx);
+
+  /* The first two bytes are server ID and run indicator.
+   * The rest is optional additional data.
+   */
+
+  if (addlen + 2 > CONFIG_NXMODBUS_REP_SERVER_ID_BUF)
+    {
+      return -ENOMEM;
+    }
+
+  pthread_mutex_lock(&ctx->lock);
+
+  ctx->server_id_len    = 0;

Review Comment:
   Please fix alignment, just one space or put all at the same column



##########
modbus/nxmodbus/core/nxmb_func_diag.c:
##########
@@ -0,0 +1,88 @@
+/****************************************************************************
+ * apps/modbus/nxmodbus/core/nxmb_func_diag.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <nuttx/compiler.h>
+
+#include <stdint.h>
+
+#include <nxmodbus/nxmodbus.h>
+
+#include "nxmb_internal.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define NXMB_DIAG_SUBFUNC_OFF       0
+#define NXMB_DIAG_DATA_OFF          2
+#define NXMB_DIAG_REQ_MIN_DATA_LEN  4
+
+/* Sub-function codes */
+
+#define NXMB_DIAG_RETURN_QUERY      0x0000
+
+/****************************************************************************
+ * Public Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: nxmb_fc08_diagnostics
+ *
+ * Description:
+ *   Handle FC08 Diagnostics request. Only sub-function 0x0000 (Return Query
+ *   Data) is supported — it echoes the request data back unchanged.
+ *

Review Comment:
   Please include Input Parameter and Return type



##########
modbus/nxmodbus/core/nxmb_func.c:
##########
@@ -0,0 +1,346 @@
+/****************************************************************************
+ * apps/modbus/nxmodbus/core/nxmb_func.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <nuttx/compiler.h>
+
+#include <errno.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <nxmodbus/nxmodbus.h>
+
+#include "nxmb_internal.h"
+
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define NXMB_EXCEPTION_FLAG 0x80
+
+/****************************************************************************
+ * Private Types
+ ****************************************************************************/
+
+/* Function code handler signature */
+
+typedef enum nxmb_exception_e (*nxmb_fc_handler_t)(nxmb_handle_t ctx);
+
+/* Dispatch table entry */
+
+struct nxmb_fc_entry_s
+{
+  uint8_t           fc;
+  nxmb_fc_handler_t handler;
+};
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+/* Standard function code dispatch table */
+
+static const struct nxmb_fc_entry_s g_nxmb_fc_table[] =
+{
+#ifdef CONFIG_NXMODBUS_FUNC_READ_COILS
+  { NXMB_FC_READ_COILS, nxmb_fc01_read_coils },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_READ_DISCRETE
+  { NXMB_FC_READ_DISCRETE, nxmb_fc02_read_discrete },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_READ_HOLDING
+  { NXMB_FC_READ_HOLDING, nxmb_fc03_read_holding },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_READ_INPUT
+  { NXMB_FC_READ_INPUT, nxmb_fc04_read_input },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_WRITE_COIL
+  { NXMB_FC_WRITE_COIL, nxmb_fc05_write_coil },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_WRITE_HOLDING
+  { NXMB_FC_WRITE_HOLDING, nxmb_fc06_write_holding },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_DIAGNOSTICS
+  { NXMB_FC_DIAGNOSTICS, nxmb_fc08_diagnostics },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_WRITE_COILS
+  { NXMB_FC_WRITE_COILS, nxmb_fc15_write_coils },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_WRITE_HOLDINGS
+  { NXMB_FC_WRITE_HOLDINGS, nxmb_fc16_write_holdings },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_REPORT_SERVER_ID
+  { NXMB_FC_REPORT_SERVER_ID, nxmb_fc17_report_server_id },
+#endif
+#ifdef CONFIG_NXMODBUS_FUNC_READWRITE_HOLDINGS
+  { NXMB_FC_READWRITE_HOLDINGS, nxmb_fc23_readwrite_holding }
+#endif
+};
+
+#define NXMB_FC_TABLE_SIZE                                                     
\
+  (sizeof(g_nxmb_fc_table) / sizeof(g_nxmb_fc_table[0]))
+
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: nxmb_build_exception
+ *
+ * Description:
+ *   Build a Modbus exception response in ctx->adu in place.
+ *

Review Comment:
   Please include the Input Parameters



##########
modbus/nxmodbus/transport/nxmb_crc.c:
##########
@@ -0,0 +1,108 @@
+/****************************************************************************
+ * apps/modbus/nxmodbus/transport/nxmb_crc.c
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.  The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.  See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+/****************************************************************************
+ * Included Files
+ ****************************************************************************/
+
+#include <nuttx/config.h>
+
+#include <nuttx/compiler.h>
+
+#include <stdint.h>
+
+#include "nxmb_internal.h"
+
+/****************************************************************************
+ * Private Data
+ ****************************************************************************/
+
+static const uint16_t g_crc16_table[256] =

Review Comment:
   I think CRC implementation could come from include/nuttx/crc16.h or a new 
header file there



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to