Skip to content

Commit ee04d19

Browse files
committed
fix(opcua): extract parse_coerce_validate helper, document missing params
Extract shared parse_coerce_validate() helper (#3 review comment): - Value parsing (bool/int/string/float dispatch), range validation, and type coercion were duplicated in handle_plc_operations, write_data, and execute_operation. The three copies had already diverged (write_data was missing data_type_hint). Now all three call parse_coerce_validate() from an anonymous namespace, keeping them in sync. Document missing parameters (#20 review comment): - Add subscription_interval_ms to the Gateway Parameters table - Document the optional ros2_topic field in node map entries - Update poll_interval_ms description to note [100, 60000] ms clamping
1 parent 39f0a64 commit ee04d19

2 files changed

Lines changed: 69 additions & 105 deletions

File tree

src/ros2_medkit_plugins/ros2_medkit_opcua/README.md

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,8 +193,19 @@ ros2_medkit_gateway:
193193
|-----------|---------|-------------|
194194
| `endpoint_url` | `opc.tcp://localhost:4840` | OPC-UA server endpoint |
195195
| `node_map_path` | (none) | Path to node map YAML (required) |
196-
| `poll_interval_ms` | `1000` | Polling interval in milliseconds |
196+
| `poll_interval_ms` | `1000` | Polling interval in ms (clamped to [100, 60000]) |
197197
| `prefer_subscriptions` | `false` | Use OPC-UA subscriptions instead of polling |
198+
| `subscription_interval_ms` | `500` | Publishing interval for OPC-UA subscriptions when `prefer_subscriptions: true` |
199+
200+
Node map entries also support an optional `ros2_topic` field to override the auto-generated ROS 2 topic name for the PLC value bridge:
201+
202+
```yaml
203+
nodes:
204+
- node_id: "ns=2;i=1"
205+
entity_id: tank_process
206+
data_name: tank_level
207+
ros2_topic: /custom/plc/tank_level # optional, overrides auto-generated /plc/tank_process/tank_level
208+
```
198209

199210
### Operation Naming Convention
200211

src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp

Lines changed: 57 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,47 @@
3030
namespace ros2_medkit_gateway {
3131

3232
namespace {
33+
34+
/// Parse a JSON "value" field, coerce to the node's declared data_type, and
35+
/// validate against the optional min/max range. Shared by handle_plc_operations,
36+
/// DataProvider::write_data, and OperationProvider::execute_operation to keep
37+
/// the three write paths in sync.
38+
tl::expected<OpcuaValue, std::string> parse_coerce_validate(const nlohmann::json & json_value,
39+
const NodeMapEntry & entry) {
40+
OpcuaValue val;
41+
try {
42+
if (entry.data_type == "bool") {
43+
val = json_value.get<bool>();
44+
} else if (entry.data_type == "int") {
45+
val = json_value.get<int32_t>();
46+
} else if (entry.data_type == "string") {
47+
val = json_value.get<std::string>();
48+
} else {
49+
val = json_value.get<double>();
50+
}
51+
} catch (const nlohmann::json::type_error &) {
52+
return tl::make_unexpected("Value type mismatch for data_type: " + entry.data_type);
53+
}
54+
55+
if (entry.has_range()) {
56+
double v = std::visit(
57+
[](auto && x) -> double {
58+
using T = std::decay_t<decltype(x)>;
59+
if constexpr (std::is_arithmetic_v<T>) {
60+
return static_cast<double>(x);
61+
}
62+
return 0.0;
63+
},
64+
val);
65+
if (v < *entry.min_value || v > *entry.max_value) {
66+
return tl::make_unexpected("Value " + std::to_string(v) + " out of range [" + std::to_string(*entry.min_value) +
67+
", " + std::to_string(*entry.max_value) + "]");
68+
}
69+
}
70+
71+
return val;
72+
}
73+
3374
bool is_valid_path_segment(const std::string & s) {
3475
if (s.empty() || s.size() > 256) {
3576
return false;
@@ -355,42 +396,13 @@ void OpcuaPlugin::handle_plc_operations(const PluginRequest & req, PluginRespons
355396
return;
356397
}
357398

358-
OpcuaValue write_val;
359-
try {
360-
if (entry->data_type == "bool") {
361-
write_val = body["value"].get<bool>();
362-
} else if (entry->data_type == "int") {
363-
write_val = body["value"].get<int32_t>();
364-
} else if (entry->data_type == "string") {
365-
write_val = body["value"].get<std::string>();
366-
} else {
367-
write_val = body["value"].get<double>();
368-
}
369-
} catch (const nlohmann::json::type_error &) {
370-
res.send_error(400, ERR_INVALID_REQUEST, "Value type mismatch for data_type: " + entry->data_type);
399+
auto parsed = parse_coerce_validate(body["value"], *entry);
400+
if (!parsed) {
401+
res.send_error(400, ERR_INVALID_REQUEST, parsed.error());
371402
return;
372403
}
373404

374-
// Range validation for safety
375-
if (entry->has_range()) {
376-
double v = std::visit(
377-
[](auto && x) -> double {
378-
using T = std::decay_t<decltype(x)>;
379-
if constexpr (std::is_arithmetic_v<T>) {
380-
return static_cast<double>(x);
381-
}
382-
return 0.0;
383-
},
384-
write_val);
385-
if (v < *entry->min_value || v > *entry->max_value) {
386-
res.send_error(400, ERR_INVALID_REQUEST,
387-
"Value " + std::to_string(v) + " out of range [" + std::to_string(*entry->min_value) + ", " +
388-
std::to_string(*entry->max_value) + "]");
389-
return;
390-
}
391-
}
392-
393-
auto write_result = client_->write_value(entry->node_id, write_val, entry->data_type);
405+
auto write_result = client_->write_value(entry->node_id, *parsed, entry->data_type);
394406
if (!write_result) {
395407
int status = (write_result.error().code == OpcuaClient::WriteError::NotConnected ||
396408
write_result.error().code == OpcuaClient::WriteError::TransportError)
@@ -408,7 +420,7 @@ void OpcuaPlugin::handle_plc_operations(const PluginRequest & req, PluginRespons
408420
[&response](auto && v) {
409421
response["value_written"] = v;
410422
},
411-
write_val);
423+
*parsed);
412424

413425
res.send_json(response);
414426
}
@@ -710,42 +722,12 @@ tl::expected<nlohmann::json, DataProviderErrorInfo> OpcuaPlugin::write_data(cons
710722
DataProviderErrorInfo{DataProviderError::InvalidValue, "Missing 'value' field in write body", 400});
711723
}
712724

713-
OpcuaValue write_val;
714-
try {
715-
if (entry->data_type == "bool") {
716-
write_val = value["value"].get<bool>();
717-
} else if (entry->data_type == "int") {
718-
write_val = value["value"].get<int32_t>();
719-
} else if (entry->data_type == "string") {
720-
write_val = value["value"].get<std::string>();
721-
} else {
722-
write_val = value["value"].get<double>();
723-
}
724-
} catch (const nlohmann::json::type_error &) {
725-
return tl::make_unexpected(DataProviderErrorInfo{DataProviderError::InvalidValue,
726-
"Value type mismatch for data_type: " + entry->data_type, 400});
727-
}
728-
729-
if (entry->has_range()) {
730-
double v = std::visit(
731-
[](auto && x) -> double {
732-
using T = std::decay_t<decltype(x)>;
733-
if constexpr (std::is_arithmetic_v<T>) {
734-
return static_cast<double>(x);
735-
}
736-
return 0.0;
737-
},
738-
write_val);
739-
if (v < *entry->min_value || v > *entry->max_value) {
740-
return tl::make_unexpected(DataProviderErrorInfo{DataProviderError::InvalidValue,
741-
"Value " + std::to_string(v) + " out of range [" +
742-
std::to_string(*entry->min_value) + ", " +
743-
std::to_string(*entry->max_value) + "]",
744-
400});
745-
}
725+
auto parsed = parse_coerce_validate(value["value"], *entry);
726+
if (!parsed) {
727+
return tl::make_unexpected(DataProviderErrorInfo{DataProviderError::InvalidValue, parsed.error(), 400});
746728
}
747729

748-
auto write_result = client_->write_value(entry->node_id, write_val, entry->data_type);
730+
auto write_result = client_->write_value(entry->node_id, *parsed, entry->data_type);
749731
if (!write_result) {
750732
auto wcode = write_result.error().code;
751733
if (wcode == OpcuaClient::WriteError::TypeMismatch) {
@@ -766,7 +748,7 @@ tl::expected<nlohmann::json, DataProviderErrorInfo> OpcuaPlugin::write_data(cons
766748
[&result](auto && v) {
767749
result["value_written"] = v;
768750
},
769-
write_val);
751+
*parsed);
770752
return tl::expected<nlohmann::json, DataProviderErrorInfo>{result};
771753
}
772754

@@ -824,42 +806,13 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string
824806
"Missing 'value' field in parameters", 400});
825807
}
826808

827-
OpcuaValue write_val;
828-
try {
829-
if (entry->data_type == "bool") {
830-
write_val = parameters["value"].get<bool>();
831-
} else if (entry->data_type == "int") {
832-
write_val = parameters["value"].get<int32_t>();
833-
} else if (entry->data_type == "string") {
834-
write_val = parameters["value"].get<std::string>();
835-
} else {
836-
write_val = parameters["value"].get<double>();
837-
}
838-
} catch (const nlohmann::json::type_error &) {
839-
return tl::make_unexpected(OperationProviderErrorInfo{
840-
OperationProviderError::InvalidParameters, "Value type mismatch for data_type: " + entry->data_type, 400});
841-
}
842-
843-
if (entry->has_range()) {
844-
double v = std::visit(
845-
[](auto && x) -> double {
846-
using T = std::decay_t<decltype(x)>;
847-
if constexpr (std::is_arithmetic_v<T>) {
848-
return static_cast<double>(x);
849-
}
850-
return 0.0;
851-
},
852-
write_val);
853-
if (v < *entry->min_value || v > *entry->max_value) {
854-
return tl::make_unexpected(OperationProviderErrorInfo{OperationProviderError::InvalidParameters,
855-
"Value " + std::to_string(v) + " out of range [" +
856-
std::to_string(*entry->min_value) + ", " +
857-
std::to_string(*entry->max_value) + "]",
858-
400});
859-
}
809+
auto parsed = parse_coerce_validate(parameters["value"], *entry);
810+
if (!parsed) {
811+
return tl::make_unexpected(
812+
OperationProviderErrorInfo{OperationProviderError::InvalidParameters, parsed.error(), 400});
860813
}
861814

862-
auto write_result = client_->write_value(entry->node_id, write_val, entry->data_type);
815+
auto write_result = client_->write_value(entry->node_id, *parsed, entry->data_type);
863816
if (!write_result) {
864817
auto wcode = write_result.error().code;
865818
if (wcode == OpcuaClient::WriteError::TypeMismatch) {
@@ -882,7 +835,7 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string
882835
[&result](auto && v) {
883836
result["value_written"] = v;
884837
},
885-
write_val);
838+
*parsed);
886839
return tl::expected<nlohmann::json, OperationProviderErrorInfo>{result};
887840
}
888841

0 commit comments

Comments
 (0)