Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FOGL-9302 Support for signed 32-bit integer values in the metric #47

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

FOGL-9302 Support for signed 32-bit integer values in the metric #47

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c53fe82
signed integer support added in proto file for Metric payload
ashish-jabble Dec 16, 2024
96aadfd
signed integer handling added in metric payload parsing
ashish-jabble Dec 16, 2024
fbd868b
Tests script updated with modified proto file
ashish-jabble Dec 16, 2024
5571d37
conflicts resolved
ashish-jabble Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion python/fledge/plugins/south/mqtt_sparkplug/mqtt_sparkplug.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,12 @@ def on_message(self, client, userdata, msg):
elif metric.HasField("float_value"):
value = metric.float_value
elif metric.HasField("int_value"):
value = metric.int_value
# TODO: FOGL-9320 For long and double values support
# Ensure that the value fits within the range of int32
if -2147483648 <= metric.int_value <= 2147483647:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the implementation outlined in the DRAFT PR (accessible here), we are addressing the handling of the metric.datatype field to determine how to properly interpret the data contained in metric.int_value.

Overview of metric.datatype

The metric.datatype indicates the type of data represented by metric.int_value. This field informs the application whether the integer value should be treated as a signed or unsigned integer.

Handling of Integer Values

  1. Protobuf Encoding: In Protocol Buffers (protobuf), both signed and unsigned integers are encoded as unsigned integers. This means that when we receive an integer value through the protobuf payload, it does not inherently carry information about whether it is signed or unsigned.

  2. Type Casting Based on datatype:

    • The application must read the metric.datatype field to determine how to interpret the metric.int_value.
    • Based on the value of metric.datatype, we can decide if the integer should be treated as a signed integer (e.g., using ctypes.c_int) or if it should remain as an unsigned integer.

Implementation Changes in the Draft PR

  • For example, if datatype indicates that the value is signed, we cast it using: 
    value = ctypes.c_int(metric.int_value).value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To cast signed integer values in Protobuf is generally not recommended. While it might work in some cases, it's not the best or most portable approach.

a) ctypes can introduce portability issues if you're targeting environments with different architectures.

b) It also adds unnecessary overhead for simple operations, like casting or type-checking, which can be done much more directly in Python.

c) Protobuf already provides a clean and efficient way to handle integer types, including signed and unsigned integers

IMO, we should use Protobuf’s own type system and avoid unnecessary casting through ctypes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution in this pull request requires changes to the auto-generated code, which seem to introduce portability issues. I'm unsure about the specifics of the portability concerns you are pointing. Could you please point me to the relevant documentation for Protocol Buffers that explains these issues and offers a foundation for my understanding?

value = metric.int_value
else:
_LOGGER.warning(f"Ignoring metric '{metric.name}' due to value is out of range for int32.")
elif metric.HasField("string_value"):
value = metric.string_value
# TODO: FOGL- 9198 - Handle other data types
Expand Down
2 changes: 1 addition & 1 deletion python/fledge/plugins/south/mqtt_sparkplug/sparkplug_b/sparkplug_b.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ message Payload {
optional MetaData metadata = 8; // Metadata for the payload
optional PropertySet properties = 9;
oneof value {
uint32 int_value = 10;
int32 int_value = 10;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not change auto generated file, this need not be changed ..

Copy link
Member Author

@ashish-jabble ashish-jabble Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary for handling signed integers. This generic MQTT SparkplugB subscriber addresses the issue for any data type, eliminating the need for ctypes

uint64 long_value = 11;
float float_value = 12;
double double_value = 13;
Expand Down
4 changes: 2 additions & 2 deletions python/fledge/plugins/south/mqtt_sparkplug/sparkplug_b/sparkplug_b_pb2.py
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 13 additions & 4 deletions tests/mqtt-pub/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
# Define your device ID
device_id = "your_device_id"


# Sparkplug payload example for DBIRTH
def create_dbirth_payload():
message = {
Expand All @@ -51,20 +52,21 @@ def create_dbirth_payload():
binary_data = payload.SerializeToString()
return binary_data


# Create DDATA payload
def create_ddata_payload():
bool_values = [True, False]
message = {
"metrics": [
{
"name": "Temperature Sensor",
"value": random.uniform(22.0, 32.0), # Float
"value": random.uniform(-22.0, 32.0), # Float
"timestamp": int(time.time()),
"type": "float"
},
{
"name": "Humidity Sensor",
"value": random.randint(45, 175), # Integer
"value": random.randint(-45, 175), # Integer
"timestamp": int(time.time()),
"type": "integer"
},
Expand All @@ -85,7 +87,6 @@ def create_ddata_payload():

# Create a Payload instance
payload = sparkplug_b_pb2.Payload()

# Populate the Payload with Metric instances
for metric in message["metrics"]:
m = payload.metrics.add() # Add a new Metric to the Payload
Expand All @@ -96,7 +97,11 @@ def create_ddata_payload():
if metric["type"] == "float":
m.float_value = metric["value"]
elif metric["type"] == "integer":
m.int_value = metric["value"]
# Ensure that the value fits within the range of int32
if -2147483648 <= metric["value"] <= 2147483647:
m.int_value = metric["value"]
else:
print(f"Ignoring metric '{m.name}' because the value is out of range for int32.")
elif metric["type"] == "string":
m.string_value = metric["value"]
elif metric["type"] == "boolean":
Expand All @@ -106,10 +111,12 @@ def create_ddata_payload():
continue
print(
f"Publishing metric: {m.name}, Type: {metric['type']}, Value: {metric['value']}, Timestamp: {m.timestamp}")

# Now you can serialize the payload or use it as needed
binary_data = payload.SerializeToString()
return binary_data


# Create DDEATH payload
def create_ddeath_payload():
message = {
Expand All @@ -133,6 +140,7 @@ def create_ddeath_payload():
binary_data = payload.SerializeToString()
return binary_data


# Callback for when the client connects
def on_connect(client, userdata, flags, rc):
print("Connected with result code " + str(rc))
Expand All @@ -141,6 +149,7 @@ def on_connect(client, userdata, flags, rc):
client.publish(DBIRTH_TOPIC, dbirth)
print("Published DBIRTH message")


# Initialize MQTT Client
mqtt_client = mqtt.Client()
mqtt_client.on_connect = on_connect
Expand Down
Loading