Giter Club home page Giter Club logo

Comments (7)

bmalinowsky avatar bmalinowsky commented on June 1, 2024

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).

from calimero-core.

tuxedo0801 avatar tuxedo0801 commented on June 1, 2024

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?

from calimero-core.

tuxedo0801 avatar tuxedo0801 commented on June 1, 2024

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?

from calimero-core.

bmalinowsky avatar bmalinowsky commented on June 1, 2024

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.

from calimero-core.

bmalinowsky avatar bmalinowsky commented on June 1, 2024

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.

from calimero-core.

tuxedo0801 avatar tuxedo0801 commented on June 1, 2024

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" ;-)

from calimero-core.

bmalinowsky avatar bmalinowsky commented on June 1, 2024

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.

from calimero-core.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.