Fix: Add a temperature sensor demo #161
Conversation
|
@aAAaqwq: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubeedge-bot
added
the
size/XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @aAAaqwq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances KubeEdge's device management capabilities by introducing a new, comprehensive temperature sensor mapper demo. This demo includes a simulated hardware device, a Modbus mapper plugin for KubeEdge, and a web-based user interface for real-time monitoring. The Modbus mapper is designed to collect and report sensor data, support control functions, and integrate with various data storage and publishing methods. This work aims to provide practical case studies for cloud-edge collaboration in device management.
Highlights
- New Temperature Sensor Demo: Introduces a comprehensive temperature sensor mapper demo for KubeEdge, encompassing a hardware simulator, a Modbus mapper, and a web-based user interface.
- Modbus Mapper Plugin: Implements a Modbus protocol-based mapper that integrates with KubeEdge for temperature sensor management, supporting data collection, status reporting, control functions, and various communication modes (TCP/RTU).
- Flexible Data Handling: Enhances data persistence and publishing capabilities by integrating support for multiple databases (InfluxDB2, MySQL, Redis, TDengine) and communication protocols (HTTP, MQTT, OpenTelemetry).
- Interactive Web UI: Provides a new web user interface for real-time monitoring of temperature sensor data, including temperature readings, switch status, and warning indicators.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive temperature sensor example for KubeEdge, including hardware simulation, a modbus mapper, and a web UI. This is a great addition to the examples. However, there are several critical issues that need to be addressed, such as the use of an invalid Go version, incorrect logic in the WebUI handlers for setting data, bugs in database handlers, and several instances of non-thread-safe code and inefficient resource management. I've provided detailed comments and suggestions to fix these issues and improve the overall quality and robustness of the example.
| func SetTemperature(c *gin.Context) { | ||
| var temperature float64 | ||
| if err := c.ShouldBindJSON(&temperature); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
| url := "http://" + IP + ":30077/api/v1/device/default/temperature-instance/temperature" | ||
| resp,err:=http.Get(url) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to set temperature"+err.Error()}) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| c.JSON(http.StatusOK, gin.H{"temperature": temperature}) | ||
| } | ||
|
|
||
| func SetSwitch(c *gin.Context) { | ||
| var Switch int | ||
| if err := c.ShouldBindJSON(&Switch); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
| url := "http://" + IP + ":30077/api/v1/device/default/temperature-instance/temperature-switch" | ||
| resp,err:=http.Get(url) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to set switch status"+err.Error()}) | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| c.JSON(http.StatusOK, gin.H{"switch status": Switch}) | ||
| } |
There was a problem hiding this comment.
The SetTemperature and SetSwitch functions are implemented incorrectly. They receive the value to be set but then perform a GET request, effectively doing nothing to change the device state. They should perform a POST/PUT request to the correct device method endpoint to update the device property. For example, SetTemperature should call an endpoint like /api/v1/devicemethod/default/temperature-instance/UpdateTemperature/temperature/{data}.
| @@ -0,0 +1,16 @@ | |||
| FROM golang:1.23.3-alpine3.19 AS builder | |||
There was a problem hiding this comment.
The Go version 1.23.3 is not a valid or released version. This will cause the Docker build to fail. Please use a valid Go version, for example, 1.22.4. This issue is also present in temperature-sensor/hardware/go.mod.
FROM golang:1.22.4-alpine3.19 AS builder
temperature-sensor/modbus/go.mod
Outdated
| @@ -0,0 +1,57 @@ | |||
| module github.com/kubeedge/modbus | |||
|
|
|||
| go 1.23.3 | |||
There was a problem hiding this comment.
1.23.3 is not a valid Go version. This will prevent the project from building. Please use a valid and released Go version, for example, 1.22.
| go 1.23.3 | |
| go 1.22 |
|
|
||
| type DeviceData struct{ | ||
| Data interface{} `json:"data"` | ||
| Warning uint `json:"warning"`//0:normal 1:Too High 2:Too Low |
There was a problem hiding this comment.
The comment for Warning is incomplete. The code also uses 3 for the sensor's OFF status. The comment should be updated to include all possible values for clarity.
| Warning uint `json:"warning"`//0:normal 1:Too High 2:Too Low | |
| Warning uint `json:"warning"`//0:normal 1:Too High 2:Too Low 3:Sensor is OFF |
temperature-sensor/README.md
Outdated
| git clone https://github.com/kubeedge/temperature-sensor | ||
| ``` |
There was a problem hiding this comment.
The git clone command points to a repository that does not exist. Since this example is part of the kubeedge/examples repository, the instructions should guide the user to clone the main repository and then navigate to this example's directory. This is misleading for users trying to run the demo.
aAAaqwq
force-pushed
the
feat-temperature-sensor-demo
branch
from
a6fc3d2 to
72c5dc5
Compare
aAAaqwq
force-pushed
the
feat-temperature-sensor-demo
branch
from
72c5dc5 to
12c5c55
Compare
aAAaqwq
changed the title
aAAaqwq
force-pushed
the
feat-temperature-sensor-demo
branch
2 times, most recently
from
9d8404f to
42568a9
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive temperature sensor demo, which is a great addition to the examples. The implementation covers hardware simulation, a modbus mapper, and a web UI. However, the review identified several critical and high-severity issues related to correctness, efficiency, and maintainability. Key problems include critical bugs in the Redis database client logic and MQTT publishing that could cause crashes or incorrect behavior. There are also significant performance issues from inefficient client management in HTTP, MQTT, and database clients. Several Dockerfiles can be optimized for smaller image sizes and faster builds, and the web UI contains JavaScript errors that will break its functionality. Addressing these points will greatly improve the quality and robustness of this valuable demo.
| opts := mqtt.NewClientOptions().AddBroker(pm.MQTT.Address) | ||
| client := mqtt.NewClient(opts) | ||
|
|
||
| if token := client.Connect(); token.Wait() && token.Error() != nil { | ||
| fmt.Println(token.Error()) | ||
| os.Exit(1) | ||
| } | ||
| formatTimeStr := time.Unix(data.TimeStamp/1e3, 0).Format("2006-01-02 15:04:05") | ||
| str_time := "time is " + formatTimeStr + " " | ||
| str_publish := str_time + pm.MQTT.Topic + ": " + data.Value | ||
|
|
||
| token := client.Publish(pm.MQTT.Topic, byte(pm.MQTT.QoS), pm.MQTT.Retained, str_publish) | ||
| token.Wait() | ||
|
|
||
| client.Disconnect(250) |
There was a problem hiding this comment.
This MQTT publishing logic has two major issues:
- Critical: Calling
os.Exit(1)on a connection failure is too drastic. It will terminate the entire mapper process. This should be handled more gracefully by logging the error and implementing a retry mechanism. - High: Creating a new MQTT client, connecting, publishing, and disconnecting for every single message is extremely inefficient and will perform poorly. The client should be created once and reused for multiple publish operations.
| klog.Errorf("init redis database client err: %v", err) | ||
| return | ||
| } | ||
| reportCycle := time.Duration(twin.Property.ReportCycle) |
There was a problem hiding this comment.
The reportCycle is calculated incorrectly. twin.Property.ReportCycle is in milliseconds, but it's being cast directly to time.Duration (which is in nanoseconds). This will result in a much shorter cycle than intended. You need to multiply it by time.Millisecond.
| reportCycle := time.Duration(twin.Property.ReportCycle) | |
| reportCycle := time.Millisecond * time.Duration(twin.Property.ReportCycle) |
| deviceData := "TimeStamp: " + strconv.FormatInt(data.TimeStamp, 10) + " PropertyName: " + data.PropertyName + " data: " + data.Value | ||
| // Add data to ordered set. If the ordered set does not exist, it will be created. | ||
| _, err := RedisCli.ZAdd(ctx, data.DeviceName, &redis.Z{ | ||
| Score: float64(data.TimeStamp), | ||
| Member: deviceData, | ||
| }).Result() | ||
| if err != nil { | ||
| klog.V(4).Info("Exit AddData") | ||
| return err | ||
| } |
There was a problem hiding this comment.
The AddData function stores data in Redis as a simple concatenated string. However, the GetDataByDeviceID function attempts to unmarshal this string as JSON, which will fail. This is a critical bug. Data should be stored in a structured format like JSON if it's meant to be parsed as such later.
| deviceData := "TimeStamp: " + strconv.FormatInt(data.TimeStamp, 10) + " PropertyName: " + data.PropertyName + " data: " + data.Value | |
| // Add data to ordered set. If the ordered set does not exist, it will be created. | |
| _, err := RedisCli.ZAdd(ctx, data.DeviceName, &redis.Z{ | |
| Score: float64(data.TimeStamp), | |
| Member: deviceData, | |
| }).Result() | |
| if err != nil { | |
| klog.V(4).Info("Exit AddData") | |
| return err | |
| } | |
| deviceData, err := json.Marshal(data) | |
| if err != nil { | |
| klog.V(4).Infof("failed to marshal data to json: %v", err) | |
| return err | |
| } | |
| // Add data to ordered set. If the ordered set does not exist, it will be created. | |
| _, err = RedisCli.ZAdd(ctx, data.DeviceName, &redis.Z{ | |
| Score: float64(data.TimeStamp), | |
| Member: deviceData, | |
| }).Result() |
| tableName := data.Namespace + "/" + data.DeviceName | ||
| legalTable := strings.Replace(tableName, "-", "_", -1) |
There was a problem hiding this comment.
The table name is constructed from namespace/deviceName, but only hyphens (-) are sanitized from the name. The slash (/) is not sanitized and will result in an invalid table name in TDEngine. You should replace slashes as well.
| tableName := data.Namespace + "/" + data.DeviceName | |
| legalTable := strings.Replace(tableName, "-", "_", -1) | |
| tableName := strings.ReplaceAll(data.Namespace+"/"+data.DeviceName, "/", "_") | |
| legalTable := strings.Replace(tableName, "-", "_", -1) |
| ) | ||
|
|
||
| const ( | ||
| IP = "127.0.0.1" |
There was a problem hiding this comment.
The IP address 127.0.0.1 is hardcoded. This makes the application difficult to configure for different environments, especially in a containerized setup where services are on different network namespaces or nodes. This should be externalized into a configuration file or an environment variable.
| @@ -0,0 +1,157 @@ | |||
| package tdengine | |||
There was a problem hiding this comment.
This file starts with a UTF-8 Byte Order Mark (BOM), which can cause issues with some Go tools and compilers. It should be removed.
| package tdengine | |
| package tdengine |
aAAaqwq
force-pushed
the
feat-temperature-sensor-demo
branch
from
42568a9 to
fc586c2
Compare
Signed-off-by: Daniel Li <[email protected]>
aAAaqwq
force-pushed
the
feat-temperature-sensor-demo
branch
from
fc586c2 to
bd4b776
Compare
3 participants
What type of PR is this?
/kind feat
What this PR does / why we need it:
To resolve issue #6315 :Optimize KubeEdge Device Management Practical Case Studies.
Which issue(s) this PR fixes:
Fixes #6315
Special notes for your reviewer:
Does this PR introduce a user-facing change?: