address issue #137

[rbg]

Adds optional charateristics to service structs and additional function ‘addOptionalCharateristics()’ to allocated.

[brutella]

Thanks for the changes.

I find it a little bit risky to define public fields the service structs which are nil by default. How does somebody know that you have to call addOptionalCharacteristics() first?

Also addOptionalCharacteristics() is private and can't be called from outside the service package.

[rbg]

Thanks for the changes.

I find it a little bit risky to define public fields the service structs which are nil by default. How does somebody know that you have to call addOptionalCharacteristics() first?

GetCharacteristics() should return the list of 'active' Characteristics, that would seem the "correct" way programmatically ?

Also addOptionalCharacteristics() is private and can't be called from outside the service package.

Yes I realized MyCamelSwereWrong :)

[brutella]

Thanks for the changes.

I find it a little bit risky to define public fields the service structs which are nil by default. How does somebody know that you have to call addOptionalCharacteristics() first?

GetCharacteristics() should return the list of 'active' Characteristics, that would seem the "correct" way programmatically ?

In most cases you access the characteristics directly.

sw := service.NewSwitch()
sw.On.SetValue(true)

---

Also I'm wondering which problem do you want solve by defining all optional characteristics for a service? In reality you can add any characteristic to a service. What's defined in the HomeKit Accessory Protocol as optional doesn't really matter. Only the required characteristics are mandatory.

[rbg]

Thanks for the changes. I find it a little bit risky to define public fields the service structs which are nil by default. How does somebody know that you have to call addOptionalCharacteristics() first?

GetCharacteristics() should return the list of 'active' Characteristics, that would seem the "correct" way programmatically ?

In most cases you access the characteristics directly.

sw := service.NewSwitch()
sw.On.SetValue(true)

could you show me an example where the characteristic is optional and i access it directly? Maybe i'm not getting something.

Also I'm wondering which problem do you want solve by defining all optional characteristics for a service? In reality you can add any characteristic to a service. What's defined in the HomeKit Accessory Protocol as optional doesn't really matter. Only the required characteristics are mandatory.

On looking a little closer I see an inconsistency. Using cmd/import it will generate a service/lightbulb.go like this..

// THIS FILE IS AUTO-GENERATED
package service

import (
    "github.com/brutella/hc/characteristic"
)

const TypeLightbulb = "43"

type Lightbulb struct {
    *Service

    On *characteristic.On
}

func NewLightbulb() *Lightbulb {
    svc := Lightbulb{}
    svc.Service = New(TypeLightbulb)

    svc.On = characteristic.NewOn()
    svc.AddCharacteristic(svc.On.Characteristic)

    return &svc
}

this is since the metadata has only On as mandatory see: https://github.com/brutella/hc/blob/master/gen/metadata.json#L3030

however: here https://github.com/brutella/hc/blob/master/service/lightbulb.go#L14 we see both mandatory and optional.

[rbg]

I think hc should be consistent with what is supported with respect to mandatory and optional. As a consumer of hc I of course think that since the structure definition is derived from the metadata is only seems reasonable and expected that both are provided for in the package service data structures.

Here is the full delta.

diff --git a/cmd/import.go b/cmd/import.go
index f750168..a4737e2 100644
--- a/cmd/import.go
+++ b/cmd/import.go
@@ -1,4 +1,4 @@
-// +build ignore
+//

 // Imports HomeKit metadata from a file and creates files for every characteristic and service.
 // It finishes by running `go fmt` in the characterist and service packages.
@@ -10,13 +10,14 @@ package main

 import (
    "encoding/json"
-   "github.com/brutella/hc/gen"
-   "github.com/brutella/hc/gen/golang"
    "io/ioutil"
    "log"
    "os"
    "os/exec"
    "path/filepath"
+
+   "github.com/brutella/hc/gen"
+   "github.com/brutella/hc/gen/golang"
 )

 var LibPath = os.ExpandEnv("$GOPATH/src/github.com/brutella/hc")
diff --git a/service/input_source.go b/service/input_source.go
index f9a6217..a299bff 100644
--- a/service/input_source.go
+++ b/service/input_source.go
@@ -14,10 +14,6 @@ type InputSource struct {
    InputSourceType        *characteristic.InputSourceType
    IsConfigured           *characteristic.IsConfigured
    CurrentVisibilityState *characteristic.CurrentVisibilityState
-   Identifier             *characteristic.Identifier
-   InputDeviceType        *characteristic.InputDeviceType
-   TargetVisibilityState  *characteristic.TargetVisibilityState
-   Name                   *characteristic.Name
 }

 func NewInputSource() *InputSource {
@@ -36,17 +32,5 @@ func NewInputSource() *InputSource {
    svc.CurrentVisibilityState = characteristic.NewCurrentVisibilityState()
    svc.AddCharacteristic(svc.CurrentVisibilityState.Characteristic)

-   svc.Identifier = characteristic.NewIdentifier()
-   svc.AddCharacteristic(svc.Identifier.Characteristic)
-
-   svc.InputDeviceType = characteristic.NewInputDeviceType()
-   svc.AddCharacteristic(svc.InputDeviceType.Characteristic)
-
-   svc.TargetVisibilityState = characteristic.NewTargetVisibilityState()
-   svc.AddCharacteristic(svc.TargetVisibilityState.Characteristic)
-
-   svc.Name = characteristic.NewName()
-   svc.AddCharacteristic(svc.Name.Characteristic)
-
    return &svc
 }
diff --git a/service/lightbulb.go b/service/lightbulb.go
index e305c1c..521b2d7 100644
--- a/service/lightbulb.go
+++ b/service/lightbulb.go
@@ -10,10 +10,7 @@ const TypeLightbulb = "43"
 type Lightbulb struct {
    *Service

-   On         *characteristic.On
-   Brightness *characteristic.Brightness
-   Saturation *characteristic.Saturation
-   Hue        *characteristic.Hue
+   On *characteristic.On
 }

 func NewLightbulb() *Lightbulb {
@@ -23,14 +20,5 @@ func NewLightbulb() *Lightbulb {
    svc.On = characteristic.NewOn()
    svc.AddCharacteristic(svc.On.Characteristic)

-   svc.Brightness = characteristic.NewBrightness()
-   svc.AddCharacteristic(svc.Brightness.Characteristic)
-
-   svc.Saturation = characteristic.NewSaturation()
-   svc.AddCharacteristic(svc.Saturation.Characteristic)
-
-   svc.Hue = characteristic.NewHue()
-   svc.AddCharacteristic(svc.Hue.Characteristic)
-
    return &svc
 }
diff --git a/service/television.go b/service/television.go
index 0e15826..b518291 100644
--- a/service/television.go
+++ b/service/television.go
@@ -14,14 +14,6 @@ type Television struct {
    ActiveIdentifier   *characteristic.ActiveIdentifier
    ConfiguredName     *characteristic.ConfiguredName
    SleepDiscoveryMode *characteristic.SleepDiscoveryMode
-   Brightness         *characteristic.Brightness
-   ClosedCaptions     *characteristic.ClosedCaptions
-   DisplayOrder       *characteristic.DisplayOrder
-   CurrentMediaState  *characteristic.CurrentMediaState
-   TargetMediaState   *characteristic.TargetMediaState
-   PictureMode        *characteristic.PictureMode
-   PowerModeSelection *characteristic.PowerModeSelection
-   RemoteKey          *characteristic.RemoteKey
 }

 func NewTelevision() *Television {
@@ -40,29 +32,5 @@ func NewTelevision() *Television {
    svc.SleepDiscoveryMode = characteristic.NewSleepDiscoveryMode()
    svc.AddCharacteristic(svc.SleepDiscoveryMode.Characteristic)

-   svc.Brightness = characteristic.NewBrightness()
-   svc.AddCharacteristic(svc.Brightness.Characteristic)
-
-   svc.ClosedCaptions = characteristic.NewClosedCaptions()
-   svc.AddCharacteristic(svc.ClosedCaptions.Characteristic)
-
-   svc.DisplayOrder = characteristic.NewDisplayOrder()
-   svc.AddCharacteristic(svc.DisplayOrder.Characteristic)
-
-   svc.CurrentMediaState = characteristic.NewCurrentMediaState()
-   svc.AddCharacteristic(svc.CurrentMediaState.Characteristic)
-
-   svc.TargetMediaState = characteristic.NewTargetMediaState()
-   svc.AddCharacteristic(svc.TargetMediaState.Characteristic)
-
-   svc.PictureMode = characteristic.NewPictureMode()
-   svc.AddCharacteristic(svc.PictureMode.Characteristic)
-
-   svc.PowerModeSelection = characteristic.NewPowerModeSelection()
-   svc.AddCharacteristic(svc.PowerModeSelection.Characteristic)
-
-   svc.RemoteKey = characteristic.NewRemoteKey()
-   svc.AddCharacteristic(svc.RemoteKey.Characteristic)
-
    return &svc
 }

[brutella]

Thanks for the changes. I find it a little bit risky to define public fields the service structs which are nil by default. How does somebody know that you have to call addOptionalCharacteristics() first?

GetCharacteristics() should return the list of 'active' Characteristics, that would seem the "correct" way programmatically ?

In most cases you access the characteristics directly.

sw := service.NewSwitch()
sw.On.SetValue(true)

could you show me an example where the characteristic is optional and i access it directly? Maybe i'm not getting something.

For example if we define the field StatusActive as optional for the ContactSensor struct and access it before calling AddOptionalCharaterics() we get an error. As a consumer of hc is might not be clear why this happens.

Also I'm wondering which problem do you want solve by defining all optional characteristics for a service? In reality you can add any characteristic to a service. What's defined in the HomeKit Accessory Protocol as optional doesn't really matter. Only the required characteristics are mandatory.

On looking a little closer I see an inconsistency. Using cmd/import it will generate a service/lightbulb.go like this..

// THIS FILE IS AUTO-GENERATED
package service

import (
  "github.com/brutella/hc/characteristic"
)

const TypeLightbulb = "43"

type Lightbulb struct {
  *Service

  On *characteristic.On
}

func NewLightbulb() *Lightbulb {
  svc := Lightbulb{}
  svc.Service = New(TypeLightbulb)

  svc.On = characteristic.NewOn()
  svc.AddCharacteristic(svc.On.Characteristic)

  return &svc
}

this is since the metadata has only On as mandatory see: https://github.com/brutella/hc/blob/master/gen/metadata.json#L3030

however: here https://github.com/brutella/hc/blob/master/service/lightbulb.go#L14 we see both mandatory and optional.

You are right. There are some exceptions to the rule that services only contain the required characteristics – for example the Lightbulb service.

But what is consistent is that if a field is defined as public in a service struct, it is initialised and not nil. If we change that we have to make sure that AddOptionalCharacteristics() is called at the right time.

[rbg]

On May 7, 2019, at 8:56 AM, Matthias notifications@github.com wrote:

You are right. There are some exceptions to the rule that services only contain the required characteristics – for example the Lightbulb service.

But what is consistent is that if a field is defined as public in a service struct, it is initialised and not nil. If we change that we have to make sure that AddOptionalCharacteristics() is called at the right time.

Sure, I get that the hc consumer has to know what they are doing insofar as if they want to use Optionals they’d have to explicitly add via a call to AddOptionalCharacteristics(). If they don’t then they should not dereference .. but i will note since all the characteristics are pointers in the service struct a careful programmer would always validate before deref.

However, I was attempting to mitigate the concern that you had with respect to pr #138 and your comments:

Optional characteristics should be optional. If people upgrade to a new version of hc, they would automatically get new characteristics attached to their accessories. Even when they don't need them.

The current service implementations only includes the required characteristics. That's because I wanted to provide only the bare minimum. If people wanted to have more characteristics, they could create their own service structs.

We have to decide to either include only the requirement characteristics (that's how it's currently done) or all characteristics.

I leaning towards the current implementation to only include the required characteristics.

which as we know is not correct since hc is inconsistent in that respect.

[rbg]

I'll fork here: github.com/grumpylabs/hcf

Thanks!