ProcessCommunicatorImpl: Not able to write a response

[tuxedo0801]

I'm using Calimero to adapt a RS485 device to KNX, so that it behaves like a KNX device.

So far, it works. But:

If someone initiates a read-request for a specific group-address the software-device is listening on, calimero is answering the request f.i. by calling ProcessCommunicatorImpl#public void write(GroupAddress, boolean).

This call is delegated to private method write(GroupAddress, Priority, DPTXlator), which invokes

lnk.sendRequestWait(dst, p, createGroupAPDU(GROUP_WRITE, t));

So, the write-calls which can be done by ProcessCommunicatorImpl are always of the WRITE. No possibility to send a RESPONSE.

I like to discuss a solution for this. For now, I copy&pasted the complete ProcessCommunicatorImpl class, duplicated the write methods and added a boolean-parameter to each duplicated public-write method: "boolean isResponse", and added "int service" to the private-write method so that in case of "isResponse=true", the service GROUP_RESPONSE is used instead of GROUP_WRITE.

So now i have the option in ProcessCommunicatorImpl to call

write(GroupAddress dst, boolean value) 

to send a WRITE, or use

write(boolean isResponse, GroupAddress dst, boolean value)

and decide whether this is a WRITE or a RESPONSE.

How to you guys think about it?

br, Alex

[bmalinowsky]

By design, the response logic is part of ProcessCommunicationResponder.java. Have you tried using that class? For a discussion maybe its best if you point out what is missing from that class wrt to your application needs, or the advantages of having the additional response code directly in the core as you show.

Related: although I don't know how your implementation looks in detail, for sending responses make sure that you don't send on the notification thread the notifications come in (or be aware of the implications).

[tuxedo0801]

Hmm, The ProcessCommunicationResponder-Class is not part of calimero-core project.

Yesterday I had a quick look at the calimero-device project. But as the description said "skeleton for ..." I did not further investigate the source.

Regarding my application:

It's not a big thing: It just listens to configurable group-addresses for write- and read-requests and forwards the requests to the real device (which only communicates via RS485) which will apply the write-data or answers the read-request.

And for the read-request, I thought I could use the ProcessCommunicatorImpl. But as I found out: sending a response is not possible with this class.

Instead of having ProcessCommunicationResponder in a separate project as a kind of skeleton: Why not name it ProcessResponseCommunicatorImpl (to name it similar to ProcessCommunicatorImpl, but have "response" in it to show the difference) and put it into the core-project? It makes no sense to have the core-project being able to process read-requests, if sending a response is not possible (out-of-the-box with the core-project).

for sending responses make sure that you don't send on the notification thread the notifications come in (or be aware of the implications).

There was no javadoc-comment on this (or I haven't found it yet). Why is this a problem?

[tuxedo0801]

Hmm, having a 2nd class that does - except the GROUP_WRITE service - the same as ProcessCommunicatorImpl is maybe not the best idea (code-duplication...).

If one can process read-requests with help of ProcessCommunicatorImpl, why not also be able to send response-messages with the same class? With other words: Can you explain this "by design" decision?

[bmalinowsky]

Yesterday I had a quick look at the calimero-device project. But as the description said "skeleton for ..." I did not further investigate the source.

Maybe that's a unfortunate wording on my side. The KNX device is a skeleton in the sense, that a user has to provide the device application logic. It's impossible to provide the implementation for specific KNX devices (obviously) ...

It just listens to configurable group-addresses for write- and read-requests and forwards the requests to the real device (which only communicates via RS485) which will apply the write-data or answers the read-request.

This sounds like the most common use-case I use the KNX device, also for testing purposes, etc. (except the RS485 part).

sending a response is not possible with this class. [ProcessCommunicatorImpl] ... Why not name it ProcessResponseCommunicatorImpl [...] and put it into the core-project? It makes no sense to have the core-project being able to process read-requests, if sending a response is not possible (out-of-the-box with the core-project). ... With other words: Can you explain this "by design" decision?

Most applications want to access a KNX network, using whatever protocol. They need to read/write data (e.g., for a home server), but do not need to take part in group communication or management (wrt answering group communication, react to management services). That's the reason any outbound communication of that kind is not available by default. One would have to put all KNX device files in the core. By the same reasoning, the KNX server is separated. Most applications are not servers. Maybe via terms of functionality: Access <- Device <- Server, which usually also acts as KNX Device. That order is not without its flaws, neither strict ;)

In a wider sense, keeping the KNX device separate allows to add generic network link implementations for KNX device platforms (right now there is none on github) without making the core dependent on it (which it shouldn't).

TL:DR main reason: keep different concerns separate.

[bmalinowsky]

for sending responses make sure that you don't send on the notification thread the notifications come in (or be aware of the implications).

There was no javadoc-comment on this (or I haven't found it yet). Why is this a problem?

Well, as I said I don't know your implementation, it might be nothing. But I have seen several people do it. The problem is not anything specific to the library or even Java, simply the fact that executing on the notification thread with listeners will defer the current notification for any listener down the listener chain, and will defer any new notification until any listener finished processing the previous notification. The worst-case scenario is that you timeout subsequent notifications from a remote communication partner.

[tuxedo0801]

Most applications want to access a KNX network, using whatever protocol. They need to read/write data (e.g., for a home server)

agree

TL:DR main reason: keep different concerns separate.

also agree

But again:

Why can I handle read-requests in core-project with default classes, when I'm not able to send a response with default-classes?

Or what's exactly the use case of beeing able to process a read-request without beeing able to sending a response (with default classes)?

From that point of view, I would say the core-project is missing the/a class that is able to send a response.

In a wider sense, keeping the KNX device separate allows to add generic network link implementations for KNX device platforms (right now there is none on github) without making the core dependent on it (which it shouldn't).

Also agree. But all which is required so send a response is already there. It's just about swichting an integer. It's not about adding tons of additional classes and create new dependencies. The user now has to almost copy&paste the ProcessCommunicatorImpl (or use the ProcessCommunicationResponder, which is almost a copy). That's neither intuitive, nor good code.

Well, as I said I don't know your implementation, it might be nothing.

Well, ok. then it's just a general "be careful what yuo do" ;-)

[bmalinowsky]

what's exactly the use case of beeing able to process a read-request without beeing able to sending a response (with default classes)?

First and foremost any kind of monitoring. One is interested in the ongoing communication (which include .reqs), without participating.

The user now has to almost copy&paste the ProcessCommunicatorImpl (or use the ProcessCommunicationResponder, which is almost a copy).

You don't need ProcessCommunicatorImpl, write service in both the KNX core and device is accessed via the interface ProcessCommunicationBase. Whether some of the code is currently copy/paste or not is an implementation detail. I can always move common code in a base type.

That's neither intuitive, nor good code.

I understand your point that the addition of writing the .res is straightforward. Having the one additional responder interface in the core is not the reason why it was left out.

IMO, in such case, the core should be evaluated based on how intuitive it is to implement a KNX device. And that's not just one interface. A device supports a minimum of management, and if you don't write a System 1 device, you have to support interface objects.

I don't object to moving some parts, but I do want that functionality is coherent.