You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2020/11/03 21:56:25 UTC

[GitHub] [plc4x] splatch opened a new pull request #201: PLC4X-252 Support passing of PlcField in subscription builder.

splatch opened a new pull request #201:
URL: https://github.com/apache/plc4x/pull/201


   This is a follow up for earlier PLC4X-252 PR which brings same semantics to subscription requests.
   
   @chrisdutz Given that you commented on JIRA issue I believe you are valid candidate for review.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [plc4x] splatch commented on pull request #201: PLC4X-252 Support passing of PlcField in subscription builder.

Posted by GitBox <gi...@apache.org>.

splatch commented on pull request #201:
URL: https://github.com/apache/plc4x/pull/201#issuecomment-818295134


   The SPI is not an valid point, cause within SPI we have things such as `PlcReader`, `PassiveDiscovery` and other implementation details which are relevant from *driver* point of view. One of very few things related to use of API is `PlcProprietaryRequest` which is different from what I am asking about.
   I do not consider calling specific field types in context of specific PLC driver a pollution. I believe string concatenation can do much more harm and requires much more testing.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [plc4x] splatch closed pull request #201: PLC4X-252 Support passing of PlcField in subscription builder.

Posted by GitBox <gi...@apache.org>.

splatch closed pull request #201:
URL: https://github.com/apache/plc4x/pull/201


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [plc4x] splatch edited a comment on pull request #201: PLC4X-252 Support passing of PlcField in subscription builder.

Posted by GitBox <gi...@apache.org>.

splatch edited a comment on pull request #201:
URL: https://github.com/apache/plc4x/pull/201#issuecomment-745244392

> If a user uses the driver module directly and directly instantiates the custom driver field types, then his code is no longer portable

That's user decision how he uses library and not ours.

Present API in 0.8 does not expose types such as `S7Connection` or `CANopenConnection` which could be used with specific field types. Even if that would be a case there is still a pooling connection which will bring yet another type to the stack.

I do instantiate fields directly in openhab bindings cause by this way I can assure that it compiles against next PLC4X releases and does not depend on changing field syntax. While PLC4X offers universal APIs the openHAB layer built on top of it provides driver and field specific configuration options. For example here you can find an updated discovery logic for one of canopen devices supported by binding:
https://github.com/ConnectorIO/connectorio-addons/commit/29c53778791796c642c9011d9adcac0e36376e39#diff-f498576d3c3ecc8a6206f7b7327ef608c0e05441b5c6ae8afcd274d5e47eebcc

Keep in mind that passing strings involves regular expressions which proved to cause minor, but still, failures. See PLC4X-56, PLC4X-194. Gluing a string with specific field syntax just to pass it to specific driver is simply redundant and subject of additional friction when migrating to future releases (when field syntax changes it goes under compiler radar).

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [plc4x] splatch commented on pull request #201: PLC4X-252 Support passing of PlcField in subscription builder.

Posted by GitBox <gi...@apache.org>.

splatch commented on pull request #201:
URL: https://github.com/apache/plc4x/pull/201#issuecomment-745250414


   Ok, code is rebased to current develop and ready for further review.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [plc4x] sruehl commented on pull request #201: PLC4X-252 Support passing of PlcField in subscription builder.

Posted by GitBox <gi...@apache.org>.

sruehl commented on pull request #201:
URL: https://github.com/apache/plc4x/pull/201#issuecomment-817078332


   2 Points reading your comment @splatch:
   
   1. @chrisdutz has a valid point with moving that to the SPI
   2. Regarding the "does it compile" comment: Maybe bind/use the fields in unit tests, then you would have a compile issue building your module on breaking changes and you don't pollute your productive code (You also could depend on internal artifacts with the test scope)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [plc4x] splatch commented on pull request #201: PLC4X-252 Support passing of PlcField in subscription builder.

Posted by GitBox <gi...@apache.org>.

splatch commented on pull request #201:
URL: https://github.com/apache/plc4x/pull/201#issuecomment-745244392

> If a user uses the driver module directly and directly instantiates the custom driver field types, then his code is no longer portable

That's user decision how he uses library and not ours.

Present API in 0.8 does not expose types such as `S7Connection` or `CANopenConnection` which could be used with specific types. Even if that would be a case there is still a pooling connection which will bring yet another type to the stack.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org