[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r213965185
##
File path:
integrations/apache-camel/src/main/java/org/apache/plc4x/camel/Plc4XConsumer.java
##
@@ -84,22 +79,29 @@ public void setExceptionHandler(ExceptionHandler
exceptionHandler) {
}
@Override
-protected void doStart() throws InterruptedException, ExecutionException,
TimeoutException {
-PlcSubscriptionRequest request = new PlcSubscriptionRequest();
-@SuppressWarnings("unchecked")
-SubscriptionRequestCyclicItem subscriptionRequestCyclicItem = new
SubscriptionRequestCyclicItem(dataType, address, TimeUnit.SECONDS, 3, this);
-request.addItem(subscriptionRequestCyclicItem);
+protected void doStart() throws InterruptedException, ExecutionException,
TimeoutException, PlcException {
+PlcSubscriber plcSubscriber =
plcConnection.getSubscriber().orElseThrow(
+() -> new PlcException("Connection doesn't support
subscriptions."));
+// TODO: Is it correct to only support one field?
+PlcSubscriptionRequest request =
plcSubscriber.subscriptionRequestBuilder()
+.addCyclicField("default", fieldQuery, Duration.of(3,
ChronoUnit.SECONDS)).build();
CompletableFuture subscriptionFuture =
getSubscriber().subscribe(request);
subscriptionResponse = subscriptionFuture.get(5, TimeUnit.SECONDS);
}
@Override
-protected void doStop() {
-PlcUnsubscriptionRequest request = new PlcUnsubscriptionRequest();
-subscriptionResponse.getResponseItems().stream()
-.map(SubscriptionResponseItem::getSubscriptionHandle)
-.map(UnsubscriptionRequestItem::new)
-.forEach(request::addItem);
+protected void doStop() throws InterruptedException, ExecutionException,
TimeoutException, PlcException {
+PlcSubscriber plcSubscriber =
plcConnection.getSubscriber().orElseThrow(
+() -> new PlcException("Connection doesn't support
subscriptions."));
+PlcUnsubscriptionRequest.Builder builder =
plcSubscriber.unsubscriptionRequestBuilder();
+// For every field we subscribed for, now unsubscribe.
+subscriptionResponse.getFieldNames().forEach(fieldName -> {
Review comment:
maybe this can be moved into the unsubscription builder so we don't repeat
that code everywhere.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r213966073
##
File path:
plc4j/api/src/main/java/org/apache/plc4x/java/api/connection/PlcProprietarySender.java
##
@@ -1,41 +0,0 @@
-/*
- Licensed to the Apache Software Foundation (ASF) under one
- or more contributor license agreements. See the NOTICE file
- distributed with this work for additional information
- regarding copyright ownership. The ASF licenses this file
- to you under the Apache License, Version 2.0 (the
- "License"); you may not use this file except in compliance
- with the License. You may obtain a copy of the License at
-
- http://www.apache.org/licenses/LICENSE-2.0
-
- Unless required by applicable law or agreed to in writing,
- software distributed under the License is distributed on an
- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- KIND, either express or implied. See the License for the
- specific language governing permissions and limitations
- under the License.
- */
-package org.apache.plc4x.java.api.connection;
-
-import org.apache.plc4x.java.api.messages.PlcProprietaryRequest;
-import org.apache.plc4x.java.api.messages.PlcProprietaryResponse;
-
-import java.util.concurrent.CompletableFuture;
-
-/**
- * Interface implemented by all PlcConnections that are able to send custom
commands to resources.
- */
-@FunctionalInterface
-public interface PlcProprietarySender {
Review comment:
why did this got deleted?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r213967016
##
File path:
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcFieldResponse.java
##
@@ -0,0 +1,42 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+package org.apache.plc4x.java.api.messages;
+
+import org.apache.plc4x.java.api.model.PlcField;
+import org.apache.plc4x.java.api.types.PlcResponseCode;
+
+import java.util.Collection;
+import java.util.Map;
+
+/**
+ * Base type for all response messages sent as response for a prior request
+ * from a plc to the plc4x system.
+ * @param
+ */
+public interface PlcFieldResponse
extends PlcResponse {
+
+Map getFields();
+
+Collection getFieldNames();
Review comment:
This should be more a `java.util.Set` as these are unique, aren't they?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r213965327
##
File path:
integrations/apache-camel/src/main/java/org/apache/plc4x/camel/Plc4XConsumer.java
##
@@ -112,25 +114,25 @@ private PlcSubscriber getSubscriber() {
}
@Override
-public void accept(SubscriptionEventItem subscriptionEventItem) {
-LOGGER.debug("Received {}", subscriptionEventItem);
+public void accept(PlcSubscriptionEvent plcSubscriptionEvent) {
+LOGGER.debug("Received {}", plcSubscriptionEvent);
try {
Exchange exchange = endpoint.createExchange();
-
exchange.getIn().setBody(unwrapIfSingle(subscriptionEventItem.getValues()));
+
exchange.getIn().setBody(unwrapIfSingle(plcSubscriptionEvent.getAllObjects("default")));
Review comment:
where does this "default" constant come from?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r213968452
##
File path:
plc4j/api/src/test/java/org/apache/plc4x/java/api/connection/PlcReaderTest.java
##
@@ -38,24 +36,24 @@ Licensed to the Apache Software Foundation (ASF) under one
public class PlcReaderTest {
-@Test
+/*@Test
Review comment:
I don't get the whole outcommenting of tests right now. It this then an open
TODO or what is the reason for this?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r213966847
##
File path:
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcFieldRequest.java
##
@@ -0,0 +1,31 @@
+/*
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+*/
+package org.apache.plc4x.java.api.messages;
+
+import org.apache.plc4x.java.api.model.PlcField;
+
+import java.util.Collection;
+
+public interface PlcFieldRequest extends PlcRequest {
+
+Collection getFieldNames();
Review comment:
This should be more a `java.util.Set` as these are unique, aren't they?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r213964940
##
File path:
integrations/apache-camel/src/main/java/org/apache/plc4x/camel/Plc4XConsumer.java
##
@@ -84,22 +79,29 @@ public void setExceptionHandler(ExceptionHandler
exceptionHandler) {
}
@Override
-protected void doStart() throws InterruptedException, ExecutionException,
TimeoutException {
-PlcSubscriptionRequest request = new PlcSubscriptionRequest();
-@SuppressWarnings("unchecked")
-SubscriptionRequestCyclicItem subscriptionRequestCyclicItem = new
SubscriptionRequestCyclicItem(dataType, address, TimeUnit.SECONDS, 3, this);
-request.addItem(subscriptionRequestCyclicItem);
+protected void doStart() throws InterruptedException, ExecutionException,
TimeoutException, PlcException {
+PlcSubscriber plcSubscriber =
plcConnection.getSubscriber().orElseThrow(
+() -> new PlcException("Connection doesn't support
subscriptions."));
+// TODO: Is it correct to only support one field?
Review comment:
it depends how the camel uri is defined. In ads and modbus I omit query
params so they can be used by camel
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r213981954
##
File path:
plc4j/api/src/test/java/org/apache/plc4x/java/api/connection/PlcReaderTest.java
##
@@ -38,24 +36,24 @@ Licensed to the Apache Software Foundation (ASF) under one
public class PlcReaderTest {
-@Test
+/*@Test
Review comment:
ok, fair enough :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r214035318
##
File path:
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcFieldRequest.java
##
@@ -20,11 +20,13 @@ Licensed to the Apache Software Foundation (ASF) under one
import org.apache.plc4x.java.api.model.PlcField;
-import java.util.Collection;
+import java.util.LinkedHashSet;
public interface PlcFieldRequest extends PlcRequest {
-Collection getFieldNames();
+int getNumFields();
+
+LinkedHashSet getFieldNames();
Review comment:
as this is an interface, is there any reason to use `LinkedHashSet`? As you
said earlier if the order is important maybe the `java.util.SortedSet` might be
more appropriate. Even the `java.util.NavigatableSet` might be an option. Sonar
might highlight the issue later anway.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r214037249
##
File path:
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcWriteRequest.java
##
@@ -24,6 +24,8 @@ Licensed to the Apache Software Foundation (ASF) under one
public interface PlcWriteRequest extends PlcFieldRequest {
+int getNumValues(String name);
Review comment:
in the java world one would name these `getNumberOfValues` even if this is a
bit more verbose but these shortcuts are not seen that much.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sruehl commented on a change in pull request #14: WIP: Api Refactoring
sruehl commented on a change in pull request #14: WIP: Api Refactoring
URL: https://github.com/apache/incubator-plc4x/pull/14#discussion_r214042283
##
File path:
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadResponse.java
##
@@ -18,40 +18,81 @@ Licensed to the Apache Software Foundation (ASF) under one
*/
package org.apache.plc4x.java.api.messages;
-import org.apache.plc4x.java.api.messages.items.ReadRequestItem;
-import org.apache.plc4x.java.api.messages.items.ReadResponseItem;
-
-import java.util.Collections;
-import java.util.List;
-import java.util.Optional;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.util.Collection;
/**
* Response to a {@link PlcReadRequest}.
- * Contains the values read from the PLC but untyped.
- *
- * Values are extracted using the {@link ReadRequestItem}s that were send in
the read request.
- *
- * If only a variables of one type are requested it is better to use
- * {@link org.apache.plc4x.java.api.messages.specific.TypeSafePlcReadRequest}
which leads to a
- * {@link org.apache.plc4x.java.api.messages.specific.TypeSafePlcReadResponse}.
*/
-public class PlcReadResponse extends PlcResponse, ReadRequestItem> {
+public interface PlcReadResponse extends PlcFieldResponse {
+
+int getNumValues(String name);
+
+boolean isRaw(String name);
Review comment:
I just thought about why not convert these methods into `is(Class clazz,
String name)`, `get(Class clazz, String name)` `getAll(Class clazz, String
name)`. From a user perspective this opens some more possibilities.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
