Modbus binding alignment#1261
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1261 +/- ##
=======================================
Coverage 77.81% 77.81%
=======================================
Files 84 84
Lines 17523 17523
Branches 1781 1781
=======================================
Hits 13635 13635
Misses 3853 3853
Partials 35 35 ☔ View full report in Codecov by Sentry. |
|
Below is a code I have used that does full TD parsing. I will edit the code based on the changes: https://gist.github.com/egekorkan/e2489da6ca5528720ea10f601c4a0166 It acts against https://github.com/eclipse-thingweb/test-things/blob/main/things/elevator/modbus/js/main.js with modified TD (in the code) but unmodified thing code |
relu91
left a comment
relu91
left a comment
There was a problem hiding this comment.
Good to go, just some minor point below. Also a question: the modv:type and brothers are not yet implemented right?
| private overrideFormFromURLPath(input: ModbusForm) { | ||
| // This generates a form with url content based on the uri scheme | ||
| // Ideally, more code should be refactored to use uri only | ||
| private generateFormFromURLPath(input: ModbusForm) { |
There was a problem hiding this comment.
I'm ok with finding a better name for this function. However, generate does not really explain the fact that the function actually modifies the form in input with the values taken from the URI. We can clone the form input and make it immutable to better fit the description.
There was a problem hiding this comment.
Good point. We are overriding the form but more that we are adding terms into it whereas before the URI values would be prioritized (no code change though). How about addFormElementsFromURLPath
There was a problem hiding this comment.
I like the idea of not changing the input form and rather clone and update. Doing so the term generate with reporting back the update is fine I think
There was a problem hiding this comment.
This is now done. Can we resolve this thread and merge the PR?
No not yet. I didn't have time for those but I can do them as a follow-up PR. |
danielpeintner
left a comment
danielpeintner
left a comment
There was a problem hiding this comment.
Find some comments below
| servient.addClientFactory(new ModbusClientFactory()); | ||
|
|
||
| async function main() { | ||
| let td = {}; // see https://github.com/eclipse-thingweb/test-things/blob/main/things/elevator/modbus/js/modbus-elevator.td.json |
There was a problem hiding this comment.
Why don't you want to include the TD ?
There was a problem hiding this comment.
It is changing at the moment due to the changes in this PR. Once this PR is merged and that TD is updated, I can add it inline. Another alternative would be to add a fetch method but that would imply adding the HTTP binding
3 participants
Fixes #1234 and #1168 (when the PR is ready ofc).