Drutol / CrossHMI

Reactive HMI Android application example
2 stars 0 forks source link

`NetworkEventsManager` is tightly coupled with ConfigurationData #7

Open mpostol opened 4 years ago

mpostol commented 4 years ago

This approach is not compliant with the architecture proposed in:

Reactive Networking Application Architecture

It has very serious consequences:

  1. It is against the separation of concerns paradigm
  2. Configuration cannot be changed separately - it is very difficult to analyze the impact scope of these changes. You said Unfortunately the library does not provide any easy way to map data onto properties which will be later used for displaying the data to the user. - my concern is if it is true. RferenceApplication contains an example of how to do it.

You said: For this reason I had to build some sort of proxy which will allow to manage the process much more easily. - my concern is how to prove it, how to measure the effort required to (not sure) to implement or to understand or both.

Drutol commented 4 years ago

It is against the separation of concerns paradigm

At the time of writing this code the assumption was that configuration is static and does not change, hence it's treated as a part of application domain. That being said it's only being used for accessing extra pieces of metadata about given instance(repository) so removing it won't have much of an impact given that in the end it should come separately after obtaining information about given repository from external sources.

Now that I think of it there will be 3 kinds of data to discover:

  1. How to decode packets.
  2. What is the meaning of the decoded data for a whole type.
  3. Specific data about the instance.

While the first two should be contained in one configuration file the third one is separate piece of data.

Configuration cannot be changed separately - it is very difficult to analyze the impact scope of these changes.

Not quite sure what you mean, separately from what? Reply to the first point may be duplicate of this.

Unfortunately the library does not provide any easy way to map data onto properties which will be later used for displaying the data to the user.

What I meant here is from pure library user standpoint, right now we have this "fountain" of data in form of IConsumerBinding interfaces. What I'm doing here is just putting next layer on top of it so that I as a user can define class which will be automatically mapped to these IConsumerBinding instances so that I don't have to do the plumbing every time I add a new model. With this tooling I can either easily create a class with:

[ProcessVariable] public double PipeX001_FTX001_Output { get; private set; }

or build one in runtime:

Properties = new Dictionary<string, BuiltInType>
{
    { "DrumX001_LIX001_Output", BuiltInType.Double},
    { "PipeX002_FTX002_Output", BuiltInType.Double}
}

and be done with it. It's just a "quality of life" feature.

mpostol commented 4 years ago

Thanks for comments. Let me distinguish two independent topics related to this part of code:

  1. IConfigurationDataFactory implementation
  2. IBindingFactory implementation

I will add comments to these topics separately.

mpostol commented 4 years ago

IConfigurationDataFactory

Try to improve your code and define separate issues targeting anyone you will consider.

Dynamic configuration coupled with discovery functionality is an independent topic that we must discuss after collecting all ideas targeting this topic.

Once more thanks for the comments. I will use them to improve the documentation. The work targeting this issue mpostol/OPC-UA-OOI#401 is conducted on the branch Configuration

mpostol commented 4 years ago

I will review the modification and report any problems in separate issues if needed. Consider this as an aggregating one. We will close it after closing all dependent issues.