Added some javadoc to S7 communication path and several todos that ma… #9
Conversation
| @@ -52,6 +64,8 @@ public boolean equals(Object o) { | |||
| return false; | |||
| } | |||
| ReadRequestItem<?> that = (ReadRequestItem<?>) o; | |||
| // TODO 01.18.18 jf: we should also call the comparison of super at least otherwise this can lead to unwanted behavior. | |||
There was a problem hiding this comment.
Aren't we doing exactly this super check 4 lines above?
There was a problem hiding this comment.
I'm sorry, i really completely missed that... you're right.
| @@ -52,6 +64,8 @@ public boolean equals(Object o) { | |||
| return false; | |||
| } | |||
| ReadRequestItem<?> that = (ReadRequestItem<?>) o; | |||
| // TODO 01.18.18 jf: we should also call the comparison of super at least otherwise this can lead to unwanted behavior. | |||
| // Perhaps we should generate a UUID or something for each ReadRequest to have a good equality comparison | |||
There was a problem hiding this comment.
Generation of UUIDs takes a lot of CPU and consumes Memory
| @@ -60,6 +66,7 @@ public boolean equals(Object o) { | |||
| return false; | |||
| } | |||
| ReadResponseItem<?> that = (ReadResponseItem<?>) o; | |||
| // TODO 01.08.18 jf: should we also use a UUID here or at least a check for equal request as this could lead to effects due to unwanted equality (same for hashCode). | |||
There was a problem hiding this comment.
Generation of UUIDs takes a lot of CPU and consumes Memory
| * This layer transforms between {@link PlcRequestContainer}s {@link S7Message}s. | ||
| * And stores all "in-flight" requests in an internal structure ({@link Plc4XS7Protocol#requests}). | ||
| * | ||
| * TODO 01.08.18, jf: is a Cleanup of the requests map necessary? Can it occur that for a request no response is generated that leads to success or failure? |
There was a problem hiding this comment.
Yes we should have some housekeeping mechanism in place that handles timeouts and connection problems. The driver implementation we are using in the Enthernet/IP driver (https://github.com/digitalpetri/ethernet-ip) has been a great inspiration on how to do this. We just haven't done it yet. Perhaps it would be a good idea to create a Jira for this?
| * Also contains {@link VarParameter#items} that hold detailed information on the "Command", e.g., | ||
| * addresses to read or to write to. | ||
| * | ||
| * TODO 01.08.18 jf: Could this be renamed to Something like Command as this seems to be the command message pattern? |
There was a problem hiding this comment.
These classes are driver-internal classes. No user should ever deal with them. The naming is correlated to names used in some of the documentation of the S7 protocol. So even if I agree that the correct name for this class from a pattern point of view might be different, I would suggest to keep it the way it is, because this way there is the correlation to the informal specs.
There was a problem hiding this comment.
Okay, the correlation makes sense.
But its good to know that my assumption was right.
It simply helped my understanding of the process to see them as that.
Hey all,
i went through the S7 Communication path to understand it for myself and by doing that I tried adding useful javadoc at several classes.
Furthermore, I added several TODOs where I observed possible problems.
I am not sure if all of this javadoc is right so before merging this PR someone should go through it carefully and check if the things I state are right.