`/config/channels/scan` エンドポイントにBS/CSチャンネルスキャン機能を追加する #79

fuji44 · 2020-11-18T20:33:48Z

このプルリクの目的は、 /config/channels/scan エンドポイントでBS/CSのチャンネルをスキャンできるように機能を拡張することです。SKYは対象外です。仕様はおおむねapiDocに書けていると思うのでそちらを参照してください。

BS/CSスキャンの追加以外に、GRスキャン時の動作が一部変わっています。

チャンネルにサービスが複数ある場合、すべてのサービスを登録するように変更
serviceId を登録するように変更
チャンネル名が空文字の場合、 ${type}${channel}:${service.serviceId} で代替するように変更

GRの動作を変えることが目的ではなく、コードをシンプルに保つためGR/BS/CSの処理をできる限り統一したことによる影響です。

影響が大きそうな1に関してですが、現行コードで1件目のサービスのみを登録している意図がいまいち理解できませんでした。GRでも1件目のサービスと異なる番組を放送するチャンネルをちらほら見かける(ＮＨＫ総合２とか)ので、現在の動作はあまりよくないかなと思い変更しました。2, 3に関してはサービスを扱うようになったので必然的にという感じです。

気がかりなこととしては、recept1の仕様？に合わせるためにBSのチャンネル文字列(例: BS01_1)を生成する処理が必要になり、これに関連して無駄に複雑になっている感があること。問題なければこのままでいきたいと思っています。

本プロジェクトにコミットするのは初なので、初歩的なところやそもそもアプローチおかしいとかあっても優しく指摘してくれると嬉しいです。:wink:

kanreisa · 2020-11-19T00:55:56Z

PR いただきましてありがとうございます！
BS/CS のスキャンは地域差が無くあまりチャンネルが変わらないため実装されてきませんでした。

とても良いコミットだと思いますので、ぜひ取り込みたいと思います。

src/client.ts の channelScan() を変更に対応できますか？

Mirakurun/src/client.ts

Lines 78 to 82 in 113a10d

    
           export interface ChannelScanOption { 
        
               type?: apid.ChannelType; 
        
               min?: number; 
        
               max?: number; 
        
           }

Mirakurun/src/client.ts

Lines 352 to 355 in 113a10d

    
           async channelScan(option?: ChannelScanOption): Promise<http.IncomingMessage> { 
        
               return await this.call("channelScan", option); 
        
           }

現行コードで1件目のサービスのみを登録している意図がいまいち理解できませんでした。

それは GR では serviceId の登録が不必要なため、スキャンしている物理 ch でサービスが1つでもあればサービスを省略して登録しているからですね。（おそらく2の実装でお気づきになられたかと思います。）

Mirakurun/src/Mirakurun/api/config/channels/scan.ts

Lines 95 to 100 in 113a10d

    
           const channelItem: config.Channel = { 
        
               name: name, 
        
               type: type, 
        
               channel: channel 
        
           }; 
        
           result.push(channelItem);

GR の場合は serviceId を省略するように変更できますか？
また、スムーズなレビューのために、テストされている環境を教えていただけますでしょうか？

Mirakurun が動作している OS / Arch
Mirakurun が動作している Docker / Node.js / NPM Version
Mirakurun の tuners.yml 設定ファイル (gist 可)
スキャン実行時の API レスポンス内容 (gist 可)

どうぞよろしくお願いします。

kanreisa

commented on: #79 (comment)

fuji44 · 2020-11-19T01:22:38Z

早速、確認ありがとうございます。修正については時間が取れたら考えてみます。
スキャン実行時の API レスポンス内容についても後程。
先に、環境周りについて回答します。

ちょっとレアな環境で使っています。Dockerは使っていません。
今回の修正であれば、影響はないと思うのですが、もう少し一般的な環境でテストしたほうが良ければ、時間見つけてやります。

$ uname -a
FreeBSD mirakurun 12.2-RC3 FreeBSD 12.2-RC3 7c4ec6ff02c(HEAD) TRUENAS  amd64
$ node -v
v14.13.0
$ npm -v
6.14.8

tuners.yml

- name: PT1-S1
  types:
    - BS
    - CS
  command: recpt1 --device /dev/ptx0.s0 --lnb 15 <channel> - -
  isDisabled: false
  decoder: arib-b25-stream-test
- name: PT1-S2
  types:
    - BS
    - CS
  command: recpt1 --device /dev/ptx0.s1 --lnb 15 <channel> - -
  isDisabled: false
  decoder: arib-b25-stream-test
- name: PT1-T1
  types:
    - GR
  command: recpt1 --device /dev/ptx0.t0 <channel> - -
  isDisabled: false
  decoder: arib-b25-stream-test
- name: PT1-T2
  types:
    - GR
  command: recpt1 --device /dev/ptx0.t1 <channel> - -
  isDisabled: false
  decoder: arib-b25-stream-test

fuji44 · 2020-11-20T02:30:09Z

serviceId について質問があります。

/channels APIのコードを軽く見たのですが、チャンネル番号からサービスをスキャンする動作はチャンネルタイプを問わず同じなんですね。
また、 /channels APIのレスポンスを見ると、serviceId の登録がない場合はスキャンしたサービスすべてが返りますが、登録がある場合は登録された serviceId のみ返る。

これらのことを考えると、GRの場合に serviceId の登録を省略するというよりは、この /config/channels/scan の結果をチャンネル単位で登録するか、サービス単位で登録するかを変えられるようにしたほうがより良い気がしますが、どうでしょうか？

そのうえでデフォルトの動作を、GRならチャンネル単位、その他ではサービス単位で登録のようにするとか。個人的にはチャンネル単位に寄せてしまっても良いような気もしますが、BSやCSではサービスが多くなりがちなので避けたいということですかね？

試しに実装してみました。細かいテストはまだですが。
master...fuji44:switch-registration-levels

kanreisa · 2020-11-20T02:47:20Z

そのうえでデフォルトの動作を、GRならチャンネル単位、その他ではサービス単位で登録のようにするとか。個人的にはチャンネル単位に寄せてしまっても良いような気もしますが、BSやCSではサービスが多くなりがちなので避けたいということですかね？

そうですね、GR 以外は有料放送が大半なのでユーザーが選択しやすくする必要があります。
またすべてを有効化してしまうとCPU時間とメモリを無駄に浪費してしまいます。

そういえば、↓の UI で有効化/無効化を管理できるので、スキャン直後は GR 以外無効化しておくオプションがあると便利ですね。（スキャン前に有効化されていたサービスはそのままで・・・）

fuji44 · 2020-11-20T03:12:12Z

そうですね、GR 以外は有料放送が大半なのでユーザーが選択しやすくする必要があります。
またすべてを有効化してしまうとCPU時間とメモリを無駄に浪費してしまいます。

確かにそうですね。

そういえば、↓の UI で有効化/無効化を管理できるので、スキャン直後は GR 以外無効化しておくオプションがあると便利ですね。（スキャン前に有効化されていたサービスはそのままで・・・）

あぁ、確かに便利そうですね。そのような処理を追加する方向で考えてみます。

これらのことを考えると、GRの場合に serviceId の登録を省略するというよりは、この /config/channels/scan の結果をチャンネル単位で登録するか、サービス単位で登録するかを変えられるようにしたほうがより良い気がしますが、どうでしょうか？

これはどう思いますか？良ければ、これベースに無効化状態での登録オプションを追加する感じで進められたらと思っています。

kanreisa · 2020-11-20T04:09:27Z

ありがとうございます。

これはどう思いますか？良ければ、これベースに無効化状態での登録オプションを追加する感じで進められたらと思っています。

はい、オプションなら良いとおもいます。
また、パラメーター名はできるだけ省略しないようにお願いします。

kanreisa · 2020-11-20T04:22:20Z

s_min

→ subChMin とか

s_max

→ subChMax とか

bs_subch_style

→ subChType とか
※ string 型, enum: ["BS"]

kanreisa · 2020-11-20T04:27:13Z

src/Mirakurun/api/config/channels/scan.ts

+                const channels = [];
+                for (const ch of range(startCh, endCh)) {
+                    for (const sCh of range(startSubch, endSubch)) {
+                        channels.push(`BS${("00" + ch).slice(-2)}_${sCh}`);


ch.toString().padStart(2, "0")

が使えます

fixed: bc7dde5

kanreisa · 2020-11-20T04:29:44Z

src/Mirakurun/api/config/channels/scan.ts

+        case common.ChannelTypes.CS:
+            startCh = startCh === undefined ? 2 : startCh;
+            endCh = endCh === undefined ? 24 : endCh;
+            return range(startCh, endCh).map((v) => `CS${v.toString(10)}`);


この toString() は省略可です

fixed: bc7dde5

fuji44 · 2020-11-20T09:17:01Z

→ subChType とか
※ string 型, enum: ["BS"]

私は日本語で「BSサブチャンネル様式」だとか「BSサブチャンネル形式」というような意味で Style としていました。この場合、 Type より Style のほうが適切のように感じるのですがどうなんでしょうか・・・。英語、あまり得意じゃないので自信を持って言えないのですが。。。

💭 言葉としての正しさは置いておくとしても、 Type という単語はプログラミング言語の中での違う意味に使われすぎているので避けたいかなぁという気持ちが少しあったり・・・。

Typeのほうがいいよって言われたらすぐなびく程度の心持です。 😅

kanreisa · 2020-11-20T09:23:07Z

src/Mirakurun/api/config/channels/scan.ts

+            startCh = startCh === undefined ? 13 : startCh;
+            endCh = endCh === undefined ? 62 : endCh;
+            return {
+                channels: range(startCh, endCh).map<string>((ch) => ch.toString(10)),


ここで <string> 定義すると著しく可読性が落ちるので L42 の関数をまともにする方がオススメです。
※ L42 は function で定義するべきです

fixed: c559cd1

kanreisa · 2020-11-20T09:24:03Z

src/Mirakurun/api/config/channels/scan.ts

+                startSubch = startSubch === undefined ? 0 : startSubch;
+                endSubch = endSubch === undefined ? 2 : endSubch;
+
+                const channels = new Array<string>();


const channels: string[] = [];

fixed: c559cd1

kanreisa · 2020-11-20T09:32:16Z

↓を見ても分かる通り ChannelType という名前で使っているので、全体の統一感を優先して書いてみました。
https://github.com/Chinachu/Mirakurun/pull/79/files#diff-30d88c4b43dac48c261e6b717c0c8ea83a602abb7e7dbcc54250ccbfe2523bf3R224

それか、たぶん useSubCh: boolean みたいな感じのほうが意味が分かりやすいかもしれません。

なんとなく他のエンドポイントとか眺めてみて、違和感が無ければ何でも良いです。 (_ は使っていません)

fuji44 · 2020-11-20T09:40:57Z

私も丁度 useSubCh: boolean のような形のほうがいいかなと思っていたところだったので、こっちでいこうと思います。

_については別プロジェクトのスタイルを何も考えずに書いちゃいました。すみません。

fuji44 · 2020-11-23T14:39:13Z

ちょっと時間が空いてしまいましたが、指摘の修正と無効状態での登録など実装してみました。

テストコード追加のためにメソッド化などしているので元のコードからはかなり違っています。少し面倒かもしれませんが、コミットごとに見ていただいたほうがわかりやすいかもしれません。

各コミットと修正内容をリストにすると以下のような感じです。

d15f201: チャンネル番号系とサブチャンネル有効化のパラメータ名を修正
02e0c69: チャンネル、サービス単位登録の切り替えパラメータをenumに変更
ad1d5df: 無効状態で登録するオプションを追加
75b4f08: チャンネル名要約処理の処理を修正 (一文字も一致しないケースなど)
72cbe8b: 既存の有効設定を引き継ぐ処理を追加 (同時に引き継がないオプションも追加)
- この処理の仕様はapiDocに書いています。見てよさそうか判断してほしいです。
516eeae: APIの修正に合わせて client.ts のオプションを修正
fc9b85d: テストコードを追加

テストコードについてですが、機能が増えてきたのでテストコードのほうが良いかと思い追加しています。異常ケース、特殊ケースなどはあまり書いていません。
どう管理していくかなどわからなかったので、一旦 test/scan.spec.js に置いています。

とりあえず、以下のような感じで都度検証しています。CI連動などは別途考える感じでお願いします。

npm run build && npx mocha --exit test/scan.spec.js

以下はAPIのレスポンス結果です。細かな部分はテストコードで検証しているのでそちらを見ていただきたいです。

変更が肥大化してレビュー負荷を上げてしまい申し訳ないですが、よろしくお願いします。

kanreisa · 2020-12-18T00:17:22Z

予定が立て込んでおり、大変時間がかかっていますが今月中のマージに向けて調整する予定です。
よろしくお願いいたします。

Improve 'config/channels/scan' API.

747e48a

kanreisa requested changes Nov 19, 2020

View reviewed changes

Restore registration process at each channel level

559d07c

kanreisa reviewed Nov 20, 2020

View reviewed changes

Refactor for optimal string usage

bc7dde5

kanreisa reviewed Nov 20, 2020

View reviewed changes

fuji44 added 12 commits November 20, 2020 18:50

Refactor for more optimal string usage

c559cd1

Refactor parameter names

d15f201

Refactor parameter to switch registration behavior

02e0c69

Method extraction

44ddb13

Added registration option in disabled state

ad1d5df

Aggregate arguments into objects

276efcc

Fix to specify invalidation option of scanConfig

7e03812

Fix name aggregation for multiple service channels

5c10b0c

Fix name summarization process

75b4f08

Add channel takeover process

72cbe8b

Modify options to fit the API changes

516eeae

Add test code

fc9b85d

Remove useless code

a8674a1

kanreisa added enhancement reviewing labels Dec 1, 2020

kanreisa changed the base branch from master to feature/3.4.0-extend-status December 22, 2020 10:21

kanreisa merged commit 80eb677 into Chinachu:feature/3.4.0-extend-status Dec 24, 2020

kanreisa removed the reviewing label Dec 24, 2020

hiradimir mentioned this pull request Dec 27, 2020

チャンネルスキャンが既存のエンドポイント/オプションで動作しなくなっている。 #82

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`/config/channels/scan` エンドポイントにBS/CSチャンネルスキャン機能を追加する #79

`/config/channels/scan` エンドポイントにBS/CSチャンネルスキャン機能を追加する #79

fuji44 commented Nov 18, 2020

kanreisa commented Nov 19, 2020

kanreisa left a comment

fuji44 commented Nov 19, 2020

fuji44 commented Nov 20, 2020

kanreisa commented Nov 20, 2020 •

edited

Loading

fuji44 commented Nov 20, 2020 •

edited

Loading

kanreisa commented Nov 20, 2020

kanreisa commented Nov 20, 2020

kanreisa Nov 20, 2020

fuji44 Nov 20, 2020

kanreisa Nov 20, 2020

fuji44 Nov 20, 2020

fuji44 commented Nov 20, 2020 •

edited

Loading

kanreisa Nov 20, 2020

fuji44 Nov 20, 2020

kanreisa Nov 20, 2020

fuji44 Nov 20, 2020

kanreisa commented Nov 20, 2020

fuji44 commented Nov 20, 2020

fuji44 commented Nov 23, 2020

kanreisa commented Dec 18, 2020

/config/channels/scan エンドポイントにBS/CSチャンネルスキャン機能を追加する #79

/config/channels/scan エンドポイントにBS/CSチャンネルスキャン機能を追加する #79

Conversation

fuji44 commented Nov 18, 2020

kanreisa commented Nov 19, 2020

kanreisa left a comment

Choose a reason for hiding this comment

fuji44 commented Nov 19, 2020

fuji44 commented Nov 20, 2020

kanreisa commented Nov 20, 2020 • edited Loading

fuji44 commented Nov 20, 2020 • edited Loading

kanreisa commented Nov 20, 2020

kanreisa commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuji44 commented Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanreisa commented Nov 20, 2020

fuji44 commented Nov 20, 2020

fuji44 commented Nov 23, 2020

kanreisa commented Dec 18, 2020

`/config/channels/scan` エンドポイントにBS/CSチャンネルスキャン機能を追加する #79

`/config/channels/scan` エンドポイントにBS/CSチャンネルスキャン機能を追加する #79

kanreisa commented Nov 20, 2020 •

edited

Loading

fuji44 commented Nov 20, 2020 •

edited

Loading

fuji44 commented Nov 20, 2020 •

edited

Loading