Updated per design review comments

Signed-off-by: Jeff James <jeff@james-online.com>

bundles/org.openhab.binding.pentair/src/main/java/org/openhab/binding/pentair/internal/discovery/PentairDiscoveryService.java

@@ -71,7 +71,7 @@ public void notifyDiscoveredThing(ThingTypeUID thingTypeUID, int id, String labe
  ThingUID thingUID = new ThingUID(thingTypeUID, bridgeUID, label);
 
  DiscoveryResult result = DiscoveryResultBuilder.create(thingUID).withBridge(bridgeUID)
- .withProperty(PARAMETER_ID, id).withLabel(label).build();
+ .withProperty(PARAMETER_ID, id).withRepresentationProperty(PARAMETER_ID).withLabel(label).build();
  thingDiscovered(result);
  logger.debug("Discovered Thing {}", thingUID);
  }

bundles/org.openhab.binding.pentair/src/main/java/org/openhab/binding/pentair/internal/handler/PentairControllerHandler.java

@@ -138,7 +138,6 @@ public void goOnline() {
  "@text/offline.configuration-error.duplicate-controller");
  } else {
  super.goOnline();
-
  }
  }
 
@@ -147,11 +146,11 @@ public void finishOnline() {
  super.finishOnline();
  actions.initialize(Objects.requireNonNull(getBridgeHandler()).getBaseActions(), getPentairID());
 
- // setup syncTimeJob to run once a day, initial time to sync is 3 minutes after controller goes online. This is
- // to prevent collision with main thread queries on initial startup
- syncTimeJob = scheduler.scheduleWithFixedDelay(this::syncTime, 3, 24 * 60 * 60, TimeUnit.MINUTES);
+ // setup syncTimeJob to run once a day. The initial syncTime is called as part of the initControllerSettings as
+ // part of the controller coming online
+ syncTimeJob = scheduler.scheduleWithFixedDelay(this::syncTime, 1, 1, TimeUnit.DAYS);
 
- scheduler.execute(() -> readControllerSettings());
+ scheduler.execute(() -> initControllerSettings());
  }
 
  public void syncTime() {
@@ -167,7 +166,7 @@ public void syncTime() {
  }
  }
 
- public void readControllerSettings() {
+ public void initControllerSettings() {
  int i;
 
  actions.getSWVersion();
@@ -184,15 +183,14 @@ public void readControllerSettings() {
 
  actions.getLightGroups();
  actions.getValves();
+ syncTime();
  }
 
  @Override
  public void goOffline(ThingStatusDetail detail) {
- ScheduledFuture<?> syncTimeJob;
-
  super.goOffline(detail);
 
- syncTimeJob = this.syncTimeJob;
+ ScheduledFuture<?> syncTimeJob = this.syncTimeJob;
  if (syncTimeJob != null) {
  syncTimeJob.cancel(true);
  }

bundles/org.openhab.binding.pentair/src/main/java/org/openhab/binding/pentair/internal/parser/PentairIntelliChlorPacket.java

@@ -91,76 +91,43 @@ public String getName() {
  * controller to the chlorinator to set the salt output to a specific level.
  */
  public int getSaltOutput() {
- if (this.getByte(ACTION) != 0x11) {
- return -1;
- }
-
- return buf[SALTOUTPUT] & 0xFF;
+ return (this.getByte(ACTION) == 0x11) ? (buf[SALTOUTPUT] & 0xFF) : -1;
  }
 
  // Salinity and LED status are sent on a packet with action is 0x12. This is sent from the chlorinator.
  public int getSalinity() {
- if (this.getByte(ACTION) != 0x12) {
- return -1;
- }
-
- return (buf[SALINITY] & 0xFF) * 50;
+ return (this.getByte(ACTION) == 0x12) ? (buf[SALINITY] & 0xFF) * 50 : -1;
  }
 
  public boolean getOk() {
- if (this.getByte(ACTION) != 0x12) {
- return false;
- }
-
- return ((buf[STATUS] & 0xFF) == 0) || ((buf[STATUS] & 0xFF) == 0x80);
+ return (this.getByte(ACTION) == 0x12) ? ((buf[STATUS] & 0xFF) == 0) || ((buf[STATUS] & 0xFF) == 0x80) : false;
  }
 
  public boolean getLowFlow() {
- if (this.getByte(ACTION) != 0x12) {
- return false;
- }
- return (buf[STATUS] & 0x01) != 0;
+ return (this.getByte(ACTION) == 0x12) ? (buf[STATUS] & 0x01) != 0 : false;
  }
 
  public boolean getLowSalt() {
- if (this.getByte(ACTION) != 0x12) {
- return false;
- }
- return (buf[STATUS] & 0x02) != 0;
+ return (this.getByte(ACTION) == 0x12) ? (buf[STATUS] & 0x02) != 0 : false;
  }
 
  public boolean getVeryLowSalt() {
- if (this.getByte(ACTION) != 0x12) {
- return false;
- }
- return (buf[STATUS] & 0x04) != 0;
+ return (this.getByte(ACTION) == 0x12) ? (buf[STATUS] & 0x04) != 0 : false;
  }
 
  public boolean getHighCurrent() {
- if (this.getByte(ACTION) != 0x12) {
- return false;
- }
- return (buf[STATUS] & 0x08) != 0;
+ return (this.getByte(ACTION) == 0x12) ? (buf[STATUS] & 0x08) != 0 : false;
  }
 
  public boolean getCleanCell() {
- if (this.getByte(ACTION) != 0x12) {
- return false;
- }
- return (buf[STATUS] & 0x10) != 0;
+ return (this.getByte(ACTION) == 0x12) ? (buf[STATUS] & 0x10) != 0 : false;
  }
 
  public boolean getLowVoltage() {
- if (this.getByte(ACTION) != 0x12) {
- return false;
- }
- return (buf[STATUS] & 0x20) != 0;
+ return (this.getByte(ACTION) == 0x12) ? (buf[STATUS] & 0x20) != 0 : false;
  }
 
  public boolean getLowWaterTemp() {
- if (this.getByte(ACTION) != 0x12) {
- return false;
- }
- return (buf[STATUS] & 0x40) != 0;
+ return (this.getByte(ACTION) == 0x12) ? (buf[STATUS] & 0x40) != 0 : false;
  }
 }

bundles/org.openhab.binding.pentair/src/test/java/org/openhab/binding/pentair/internal/PentairHeatStatusTest.java

@@ -69,8 +69,6 @@ void test() {
  PentairStandardPacket p = new PentairStandardPacket(packets[0], packets[0].length);
  hs.parsePacket(p);
 
- logger.info(hs.toString());
-
  assertThat(hs.poolSetPoint, equalTo(85));
  assertThat(hs.poolHeatMode, equalTo(PentairHeatStatus.HeatMode.SOLAR));
  assertThat(hs.spaSetPoint, equalTo(94));

bundles/org.openhab.binding.pentair/src/test/java/org/openhab/binding/pentair/internal/PentairParserTest.java

@@ -91,11 +91,6 @@ public void tearDown() throws Exception {
 
  @Test
  public void test() throws InterruptedException {
- java.lang.System.setProperty("org.slf4j.simpleLogger.defaultLogLevel", "DEBUG");
-
- logger.debug("debug");
- logger.info("info");
-
  ByteArrayInputStream inputStream = new ByteArrayInputStream(stream, 0, stream.length);
 
  parser.setInputStream(inputStream);