[tryspaceorg/tryspace-lab#26] EPS #3
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new ZMQ-based I2C communication system to replace the previous callback-based approach. The changes introduce device-oriented I2C operations with direct ZMQ socket communication between nodes.
Key changes:
- Replaces callback-based I2C implementation with ZMQ socket-based device communication
- Updates test suite to reflect new I2C device-oriented API
- Standardizes comment formatting across UART and I2C headers
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/simulith_i2c.c |
Complete rewrite implementing ZMQ-based I2C device communication with init/read/write/transaction/close operations |
include/simulith_i2c.h |
Updated API to use device structures instead of callbacks, added transaction support |
test/test_i2c.c |
Rewrote tests for new device-based API, testing initialization, operations, and error handling |
src/simulith_uart.c |
Minor comment formatting changes |
include/simulith_uart.h |
Minor comment formatting and whitespace cleanup |
Makefile |
Improved component discovery logic for shared library copying |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| return i2c_buses[bus_id].read_cb(addr, reg, data, len); | ||
| simulith_log("I2C RX[%s]: %d bytes\n", device->name, rc); | ||
| return rc; |
There was a problem hiding this comment.
The zmq_recv function may return more data than the buffer can hold if the incoming message is larger than 'len'. Consider checking the message size first using zmq_msg_size() or handling potential buffer overflow.
| return rc; | |
| zmq_msg_t msg; | |
| if (zmq_msg_init(&msg) != 0) { | |
| simulith_log("simulith_i2c_read: zmq_msg_init failed\n"); | |
| return SIMULITH_I2C_ERROR; | |
| } | |
| int rc = zmq_msg_recv(&msg, device->zmq_sock, ZMQ_DONTWAIT); | |
| if (rc < 0) { | |
| zmq_msg_close(&msg); | |
| if (zmq_errno() == EAGAIN) { | |
| // No data available, non-blocking | |
| return 0; | |
| } | |
| simulith_log("simulith_i2c_read: zmq_msg_recv failed (peer may be unavailable)\n"); | |
| return SIMULITH_I2C_ERROR; | |
| } | |
| size_t msg_size = zmq_msg_size(&msg); | |
| size_t copy_size = msg_size < len ? msg_size : len; | |
| memcpy(data, zmq_msg_data(&msg), copy_size); | |
| zmq_msg_close(&msg); | |
| simulith_log("I2C RX[%s]: %zu bytes (received %zu, copied %zu)\n", device->name, msg_size, msg_size, copy_size); | |
| return (int)copy_size; |
| } | ||
| zmq_close(device->zmq_sock); | ||
| zmq_ctx_term(device->zmq_ctx); | ||
| device->init = 0; |
There was a problem hiding this comment.
Using a magic number (0) instead of a defined constant for the uninitialized state. Consider defining SIMULITH_I2C_UNINITIALIZED or similar constant for consistency with SIMULITH_I2C_INITIALIZED.
| return SIMULITH_I2C_ERROR; | ||
| } | ||
| if (strlen(device->name) > 0) { | ||
| zmq_setsockopt(device->zmq_sock, ZMQ_IDENTITY, device->name, strlen(device->name)); |
There was a problem hiding this comment.
The return value of zmq_setsockopt is not checked. If setting the socket identity fails, it could lead to unexpected behavior in peer identification.
| zmq_setsockopt(device->zmq_sock, ZMQ_IDENTITY, device->name, strlen(device->name)); | |
| int rc_id = zmq_setsockopt(device->zmq_sock, ZMQ_IDENTITY, device->name, strlen(device->name)); | |
| if (rc_id != 0) { | |
| simulith_log("simulith_i2c_init: Failed to set ZMQ_IDENTITY for '%s'\n", device->name); | |
| zmq_close(device->zmq_sock); | |
| zmq_ctx_term(device->zmq_ctx); | |
| return SIMULITH_I2C_ERROR; | |
| } |
No description provided.