Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update configuration APIs #6

Merged
merged 9 commits into from
Mar 22, 2023
Merged

Update configuration APIs #6

merged 9 commits into from
Mar 22, 2023

Conversation

akif999
Copy link
Owner

@akif999 akif999 commented Feb 13, 2023

#2 (comment)

  • (3) ReadConfig の戻り値を type Config []byte とする
    • Config 型に Stringer を実装する
    • Config 型に対して paramsToBytes を実装する
  • 主な変更点
    • config.go を追加
      • 新たに Config 型を定義
      • paramsToBytes を config.go へ移動
      • bytesToParams を追加
      • type Config に String() を実装
    • e220.go
      • ReadRegister()
        • read 時の先頭 3byte (cmd + addr + length) を除いて []byte を返すように変更
          • 先頭の情報は API 使用者は既に持っている情報であり不要なため
      • writeRegister()
        • 処理完了後に 50ms wait するように変更
          • 8a6cb9c での変更を一部戻した
          • レジスタアクセスの後、すぐに再度レジスタアクセスをすると失敗するため待つ必要があるため
      • WriteConfig() / ReadConfig()
        • []byte でなく Config 型を扱うように変更
    • examples/e220/conf/main.go
      • 変更した WriteConfig() / ReadConfig に合わせて更新

@akif999 akif999 force-pushed the update_config_if branch 2 times, most recently from 3b68c11 to 0eff0c5 Compare February 13, 2023 01:42
@sago35
Copy link
Collaborator

sago35 commented Feb 13, 2023

func (d *Device) WriteConfig(cfg Config) error で実施している各種エラーチェックは、 type Config 側で実施した方がよさそうだがうまく言語化できない。

e220/params.go Outdated
Comment on lines 24 to 34
var uartSerialPortRateStrings map[uint8]string = map[uint8]string{
UartSerialPortRate1200Bps: "UartSerialPortRate1200Bps",
UartSerialPortRate2400Bps: "UartSerialPortRate2400Bps",
UartSerialPortRate4800Bps: "UartSerialPortRate4800Bps",
UartSerialPortRate9600Bps: "UartSerialPortRate9600Bps",
UartSerialPortRate19200Bps: "UartSerialPortRate19200Bps",
UartSerialPortRate38400Bps: "UartSerialPortRate38400Bps",
UartSerialPortRate57600Bps: "UartSerialPortRate57600Bps",
UartSerialPortRate115200Bps: "UartSerialPortRate115200Bps",
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この書き方の場合、 RAM に乗ってしまう (208 byte) ので、 switch case で書いていったほうが良い (※) のかも。
※本当かどうか要調査

Copy link
Owner Author

@akif999 akif999 Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

調査のために map[uint8]string に要素を一つ追加して RAM が増加するかを調査した。

結論: 以下の変更により RAM のサイズは変化しなかった
→ map には cap があるためこうなる

(以下 examples/e220/conf のプログラムを build)


$ tinygo build --size=short -target=feather-m4-can
   code    data     bss |   flash     ram
  76056    3024    6920 |   79080    9944

$ git diff
diff --git a/e220/params.go b/e220/params.go
index a904e83..20aa2a4 100644
--- a/e220/params.go
+++ b/e220/params.go
@@ -159,4 +159,5 @@ var worCycleSettingString map[uint8]string = map[uint8]string{
        WorCycleSetting3000ms: "WorCycleSetting3000ms",
        WorCycleSetting3500ms: "WorCycleSetting3500ms",
        WorCycleSetting4000ms: "WorCycleSetting4000ms",
+       0xFF:                  "test",
 }

$ tinygo build --size=short -target=feather-m4-can
   code    data     bss |   flash     ram
  76056    3024    6920 |   79080    9944

Copy link
Owner Author

@akif999 akif999 Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

↑とは逆に1要素削除すると RAM のサイズが減少した。
よって、map を定義しないことで RAM の削減ができるため、RAM に情報を置かない方法で実装するように変更する。

$ git diff

$ tinygo build --size=short -target=feather-m4-can
   code    data     bss |   flash     ram
  76056    3024    6920 |   79080    9944

$ git diff
diff --git a/e220/params.go b/e220/params.go
index a904e83..5156b07 100644
--- a/e220/params.go
+++ b/e220/params.go
@@ -158,5 +158,4 @@ var worCycleSettingString map[uint8]string = map[uint8]string{
        WorCycleSetting2500ms: "WorCycleSetting2500ms",
        WorCycleSetting3000ms: "WorCycleSetting3000ms",
        WorCycleSetting3500ms: "WorCycleSetting3500ms",
-       WorCycleSetting4000ms: "WorCycleSetting4000ms",
 }

$ tinygo build --size=short -target=feather-m4-can
   code    data     bss |   flash     ram
  76040    2940    6920 |   78980    9860

Copy link
Owner Author

@akif999 akif999 Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map を Stringer method 内へ移しただけの ver 3c057a5
RAM から code への移動はできたが、map がスタックに置かれると考えられるため、switch 文へ変更する。

$ tinygo build --size=short -target=feather-m4-can
   code    data     bss |   flash     ram
  77496    1612    6920 |   79108    8532

Copy link
Owner Author

@akif999 akif999 Feb 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

↑を switch 文にした結果が以下。 以下の commit にて変更に対応した。
(Stringer method 以外でも map を使用しているコードがあったため、同様に変更した)
d3036ab
2ad2bc8
167edbe

$ tinygo flash --size=short -target=feather-m4-can
   code    data     bss |   flash     ram
  76328    1612    6920 |   77940    8532

@akif999
Copy link
Owner Author

akif999 commented Feb 14, 2023

#6 (comment)
Config のバリデーションをどこでどうするべきかについて検討した結果。

以下より、案 3 を採用する。

func (d *Device) WriteConfig(cfg Config) error で実施している各種エラーチェックは、
type Config 側で実施した方がよさそうだがうまく言語化できない。

validation は Config に持たせたほうが良い気もするんだけど、そうじゃない気もする。
byteTo とか toByte とかやってる分には Validation は要らんのでは?という気持ちはある。

RssiAmbient とか、パラメータ毎に個別に設定するときは validation してもいいと思うけど。
でも、メンバーに直接アクセスできる現状だとその手のチェックはできないし、わざわざメソッド化してまでやりというと。。。

  • 現状
    • WriteConfig() にて引数で受け取った Config 型の各値をチェックしている
  • 変更案
    1. validation 処理自体を削除する
      • メリット
        • 変更規模が小さい
        • 設計が簡単
      • デメリット
        • 不正なパラメーターが与えられた場合に、無視して有効なビットのみを使用する動作となる
          • ユーザーにパラメーターが不正であることを知らせられなくなる
    2. Config 型のメンバーそれぞれへの Set メソッドを作成してそれらで validation を実施する
      • メリット
        • validation を実施してユーザーにパラメーターが不正であることを伝えられる(変更前と同じ)
      • デメリット
        • 他の実装の体系に比べて大きな実装になる(?)
    3. Config 型に validation を実施するメソッドを持たせる
      • メリット
        • 変更規模が小さい
        • 設計が簡単
        • validation を実施してユーザーにパラメーターが不正であることを伝えられる(変更前と同じ)
      • デメリット
        • 変更前とさほど設計に変わりがない(改善度合いが少ない)

@akif999
Copy link
Owner Author

akif999 commented Feb 16, 2023

#6 (comment)

Config のバリデーションをどこでどうするべきかについて検討した結果。
以下より、案 3 を採用する。

案 3 に基づき以下にて変更した。
7e3740a
c998f6a

  • 備考
    • Config.Validation() は export とした
      • ユーザーが任意のタイミングで作成した E220 の設定値を validation したい場合もあると考えたため
    • ‘Config.errors‘ の各値に対応する状態 (やや煩雑で良くない設計だと考えている)
      • nil → Config.Validate() が一度もコールされていない
      • 0 → Config.Validate() をコールした結果、不正なパラメータはなかった
      • !0 → Config.Validate() をコールした結果、不正なパラメータはあった

@akif999 akif999 merged commit c998f6a into e220 Mar 22, 2023
@akif999 akif999 deleted the update_config_if branch March 22, 2023 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants