已合并
耳机厂商判断逻辑优化 #319
已合并
Ho创建于 18 天前
Ho
18 天前

一、内容说明(相关的Issue)

二、建议测试周期和提测地址

建议测试完成时间:xxxx.xx.xx
投产上线时间:xxxx.xx.xx
提测地址:CI环境/压测环境
测试账号:

三、变更内容

  • 3.1 关联PR列表

  • 3.2 数据库和部署说明

    1. 常规更新
    2. 重启unicorn
    3. 重启sidekiq
    4. 迁移任务:是否有迁移任务,没有写 "无"
    5. rake脚本:bundle exec xxx RAILS_ENV = production;没有写 "无"
  • 3.4 其他技术优化内容(做了什么,变更了什么)

    • 重构了 xxxx 代码
    • xxxx 算法优化
  • 3.5 废弃通知(什么字段、方法弃用?)

  • 3.6 后向不兼容变更(是否有无法向后兼容的变更?)

四、研发自测点(自测哪些?冒烟用例全部自测?)

自测测试结论:

五、测试关注点(需要提醒QA重点关注的、可能会忽略的地方)

检查点:

需求名称 是否影响xx公共模块 是否需要xx功能 需求升级是否依赖其他子产品
xxx 否 需要 不需要

接口测试:

性能测试:

并发测试:

其他:

likedislike
Pull Request已成功合入, 合并人@openharmony_ci
(感谢 Ho 的贡献)
openharmony_ciopenharmony_ci成员
18 天前 添加了label:waiting_on_author
HHo
18 天前 关联了issue:[Bug]:耳机厂商判断逻辑优化
openharmony_ci
openharmony_ci成员
18 天前 评论:

感谢提交 Pull Requests!如果您提交的PR已经开发完毕,请评论 "code review" 请求检视 ,更多交互操作,请访问OpenHarmony社区支持命令清单。如果需要调整订阅PR、Issue的变更状态,请访问订阅链接。


Thanks for submitting the pull request. If your Pull Request has already been developed, you can leave a "code review" comment to trigger the code review process. For more commands, please visit OpenHarmony Command List. If you need to change the subscription of a Pull Request or Issue, please visit the link.

likedislike
openharmony_ciopenharmony_ci成员
18 天前 添加了label:dco检查成功
lilong32
lilong32成员
15 天前 评论:

检视意见 · PR !319「耳机厂商判断逻辑优化」

概览:C 0 条(必须改)| W 1 条(需修复或说明理由)| S 3 条(建议采纳)| 变更 2 文件 +19 / −12(检视范围 2 个源文件 +19 / −12)

处理要求:必须改 → 修完推送后再合入;需修复或说明理由 → 修复,或说明不改的理由;建议采纳 → 采纳,或说明不采纳的理由。

一句话结论:判断方式从逐个成员收成整组,方向对,也拦住了记录缺失造成的误下发;但组内只要有一只私有耳机就整组都不再下发,混合组里本来该下发的那一只也一并不再下发 —— 这种混合情形是否为预期,请你确认。

这个 PR 干了什么:原来更新合作集与新建合作集两处各写一段循环逐个成员判断是否为私有耳机,现在合成一个公共函数:先看组内有没有私有耳机,只要有一只,整组都不再下发通用对端设备类型。共 2 个文件、19 行新增 / 12 行删除。

需要你做什么:W1 请确认混合合作集(一只私有、一只厂商记录已被清掉)在真实产品里是否存在、整组跳过是否为预期 —— 修,或说明不改的理由;S1 请回复新增声明与循环是否按 const 改 —— 采纳,或说明不采纳的理由;S2 请回复聚合循环是否改为命中即退出 —— 采纳,或说明不采纳的理由;S3 请回复是否补混合组用例,或给出人工验证步骤 —— 采纳,或说明不采纳的理由。

🔴 必须改(0 条)

无。没有发现会导致崩溃、越界、资源泄漏、权限绕过或数据损坏的问题。

🟡 需修复或说明理由(1 条)

W1 | services/service/src/cdsm/CdsmService.cpp:583 —— 组内一只私有耳机,整组都不再下发对端设备类型

  • 描述(一句话简述): 判断从逐个成员改为整组任一,混合组里普通那一只也不再下发。
  • 问题(发生了什么):
    现象:判断方式从逐个成员改为整组任一,混合组里本来该下发的那一只也不再下发。
    触发条件:同一合作集内一只耳机的厂商记录还在、另一只已经被清掉。
    改动前对照:改动前对另一只仍然会下发通用对端设备类型。
     583 | void CdsmService::SetPeerDeviceTypeToControllerByVendor(CdsmCallBackData &dataBlock, const RawAddress &reportAddr)
     585 |     bool isVendor = false;
     586 |     for (auto &memInfo : dataBlock.memberInfo) {
     588 |         isVendor |= IsVendorDevice(memInfo.memberAddr);   // ① 任一只私有即为真,其余成员照样查
     589 |     }
     590 |     NL_CHECK_RETURN(!isVendor, "[cdsm Serice]private device has set device type to controller");   // ② 为真则整组返回
     592 |     for (auto memInfo : dataBlock.memberInfo) {
     593 |         if (memInfo.memberAddr != reportAddr) {   // ③ 上面为真时,这一整段不再执行
     594 |             SetPeerDeviceTypeToControllerInner(memInfo.memberAddr);
    
  • 风险(影响):
    谁会遇到:同一合作集里既有私有耳机、又有记录已被清掉的成员,互换使用或恢复出厂后就会出现。
    后果:那只成员拿不到通用对端设备类型,是否因此走不同分支取决于仓库外的厂商实现。
    严重度:需修复或说明理由。取值变化在代码层确定,影响大小要厂商实现才能定。
    升级条件:现场出现设备类型缺失的功能差异时升一级。
  • 必要性(要不要改): 需修复或说明理由:整组跳过拦住了记录缺失造成的误下发,但组内真实的普通成员也一并不再下发。
    请回一句结论:这种混合情形在产品里是否存在、整组跳过是否为预期;另一处只按报告地址判断的地方是否一并统一。
  • 修复方法(怎么做):
    混合情形成立且要保留普通成员的下发时走方案 B;只求判断一致则走方案 A。
    方案 B 在公共函数内区分「记录缺失」与「记录为非私有」,只跳过记录缺失的成员:
    // 非当前代码:示意按成员区分「记录缺失」
    for (const auto &memInfo : dataBlock.memberInfo) {
        if (memInfo.memberAddr == reportAddr) {
            continue;
        }
        if (isVendor && !HasManufacturerBusiness(memInfo.memberAddr)) {   // 新增查询:本成员是否有厂商记录
            continue;
        }
        SetPeerDeviceTypeToControllerInner(memInfo.memberAddr);
    }
    
    建议自检:上下文(只改公共函数内判断,不动线程与锁);符号(新增查询需同步补测试桩)。
    行为(组内全普通时下发集合不变);副作用(多一次内存查表,不新增跨队列等待)。
  • 请回复:已修复 / 不改的理由

🟢 建议采纳(3 条)

S1 | services/service/src/cdsm/CdsmService.h:191 —— 新函数入参没加 const,循环还按值拷贝

  • 描述(一句话简述): 合作集入参声明成可修改引用,函数体内其实一处都没改过。
  • 问题(发生了什么):
    现象:新增函数把合作集入参声明成可修改的引用,函数体内其实一处都没改过。
    触发条件:每次调用都会发生,属代码风格与效率层面,不影响现网行为。
    改动前对照:同一类里相邻的两个声明都加了 const,新声明是唯一例外。
     191 |     void SetPeerDeviceTypeToControllerByVendor(CdsmCallBackData &dataBlock, const RawAddress &reportAddr);   // ① 入参未加 const
     586 |     for (auto &memInfo : dataBlock.memberInfo) {   // ② 同一函数内的另一种写法
     592 |     for (auto memInfo : dataBlock.memberInfo) {   // ③ 按值拷贝
    
  • 风险(影响):
    谁会遇到:后续维护这段代码的人。
    后果:可改的入参容易让人以为改它能影响合作集状态,实际改的是栈上的副本,白改一遍。
    严重度:建议采纳。没有当前功能问题,属风格与效率层面。
  • 必要性(要不要改): 建议采纳:只改一处形参与一处循环写法,成本在几行以内,还能与相邻声明保持一致。
  • 修复方法(怎么做): 声明、实现与内层循环一并按 const 改:
    // 非当前代码:CdsmService.h:191 与 CdsmService.cpp:583 同步改为 const 引用
    void SetPeerDeviceTypeToControllerByVendor(const CdsmCallBackData &dataBlock, const RawAddress &reportAddr);
    // CdsmService.cpp:592
    for (const auto &memInfo : dataBlock.memberInfo) {
    
    建议自检:上下文(不依赖调用线程);符号(只改限定符,两个调用点无需改动);行为(不新增分支);副作用(无)。
  • 请回复:已采纳 / 不采纳的理由

S2 | services/service/src/cdsm/CdsmService.cpp:585 —— 聚合循环每次都把全部成员查一遍,查询次数比改动前多

  • 描述(一句话简述): 每个成员各查一次跨队列入口,判断已成立也不提前退出。
  • 问题(发生了什么):
    现象:聚合循环对每个成员各发起一次跨队列查询,判断已经成立也不提前退出。
    触发条件:每次合作集更新或新建都会走到,成员越多开销越大。
    改动前对照:改动前每个成员只查一次;现在普通组要查两轮,成员数为 K 时共 2K−1 次。
     585 |     bool isVendor = false;
     586 |     for (auto &memInfo : dataBlock.memberInfo) {
     588 |         isVendor |= IsVendorDevice(memInfo.memberAddr);   // ① 每次都跨队列查一次,且不短路
     589 |     }
    
  • 风险(影响):
    谁会遇到:成员数多、或设备管理队列繁忙的现场。
    后果:这段逻辑跑在连接主流程的队列里,查询变多会拉长整体处理时间,但不会互相等待。
    严重度:建议采纳。本轮没有发现两条队列互相等待,属新增开销。
  • 必要性(要不要改): 建议采纳:判断一旦成立就没必要再查后面的成员,改成命中即退出即可,几行改动。
    若完整遍历是为规避记录在查询中途变化,请说明理由,我方按建议采纳保留。
  • 修复方法(怎么做): 把聚合循环改为命中即退出,判断结果不变:
    // 非当前代码:CdsmService.cpp:585-589
    bool isVendor = false;
    for (const auto &memInfo : dataBlock.memberInfo) {
        if (IsVendorDevice(memInfo.memberAddr)) {
            isVendor = true;
            break;   /* 组级判定已成立,后面成员的查询不影响结果 */
        }
    }
    
    建议自检:上下文(无调用线程上下文依赖);符号(IsVendorDevice 签名不变)。
    行为(聚合结果不变);副作用(阻塞次数只减不增)。
  • 请回复:已采纳 / 不采纳的理由

S3 | services/service/src/cdsm/CdsmService.cpp:590 —— 新增的整组跳过分支没有单测覆盖

  • 描述(一句话简述): 新分支在现有测试里跑不到,混合组这个核心场景也没有用例。
  • 问题(发生了什么):
    现象:新加的整组跳过分支在现有测试里跑不到,混合组这个核心场景也没有用例。
    触发条件:测试用的厂商类型桩恒返回 0,「是私有耳机」在测试里永远不成立。
    改动前对照:改动前没有这条分支,现有用例只覆盖全普通这一路。
    影响面:本次改动 2 个文件,没有带测试改动。
     590 |     NL_CHECK_RETURN(!isVendor, "[cdsm Serice]private device has set device type to controller");   // ① 新增分支,测试跑不到
     191 | int SleRemoteDeviceAdapter::GetManufacturerBusinessType(const RawAddress &device)   // ② 测试桩
     194 |     return 0;   // ③ 恒返回 0;私有类型值为 0x01,故该判断恒不成立
    
  • 风险(影响):
    谁会遇到:后续维护这段判断的人。
    后果:若判断写反、聚合顺序被改,现网行为会翻转而测试仍然全绿。
    严重度:建议采纳。当前代码本身没问题,缺的是回归保护。
  • 必要性(要不要改): 建议采纳:给测试桩加按地址返回私有类型的开关,再补两个用例,量级在几十行以内。
  • 修复方法(怎么做):
    一是测试桩按地址条件返回私有类型,默认仍返回 0,既有用例不受影响;
    二是补两个用例:混合组(一只私有、一只普通)期望全都不下发;全普通组期望除报告地址外全部下发。
    建议自检:上下文(只改测试侧);符号(复用现有测试桩,不新增生产符号)。
    行为(不改生产代码);副作用(按地址条件化,用例之间不共享状态)。
  • 请回复:已采纳 / 不采纳的理由

本轮结论

本轮为首次检视,4 条意见均为新增,没有历史答复需要核实。W1 最想请你看的是「混合合作集在真实产品里是否存在、整组跳过是否为预期」这一点;S1–S3 三条建议请逐条回复采纳或不采纳的理由;你答复后我们按代码逐条核实并更新结论。

检视方式(摘要)

  • 本次改动集中在 cdsm 模块一个实现文件与一个头文件,按单任务综合检视,结论另做了一次独立复核;
  • 覆盖本次全部 2 个文件、31 行改动,并回溯了两处调用点、公共函数的整组判断与新声明的全部使用位置;
  • 本轮共 4 条意见(C 0 / W 1 / S 3)。逐行依据与完整检视过程见随提醒邮件附上的报告。
likedislike
HHo
15 天前 强制推送  1 个提交:8e87f554-耳机厂商判断逻辑优化
HHo
15 天前 强制推送  1 个提交:17c886c7-耳机厂商判断逻辑优化
HHo
15 天前 强制推送  1 个提交:c9eb1b54-耳机厂商判断逻辑优化
HHo
15 天前 强制推送  1 个提交:1f734c30-耳机厂商判断逻辑优化
lilong32
lilong32成员
14 天前 评论:

检视意见复查结论 · PR !319「耳机厂商判断逻辑优化」

结论:首轮 4 条已逐条核实完毕(复查版本 1f734c30,行号一律以该版本为准)—— 2 条已核实通过、0 条撤回、2 条仍有遗留(一条等你回一句结论,一条补齐用例即可)。

需要你做什么:W1 请回一句结论(混合情形是否存在、整组跳过是否为预期);S3 请按建议补齐能真正跑到整组跳过分支的用例,或说明不采纳的理由。

需要补修 / 继续处理(2 条)

W1 | services/service/src/cdsm/CdsmService.cpp:593 —— 混合合作集里记录被清掉的成员仍整组拿不到通用对端类型,需你确认是否为预期

  • 现象:这一轮你把组内判断改成了命中即停(:588-591),判断结果与逐个查完一致,没有改变整组跳过的结论。
    :593 仍是「组内只要有一只私有耳机就整组返回」,:595-596 的下发条件也没动。
    所以上一轮问你的那件事仍然成立:同一合作集里一只耳机的厂商记录还在、另一只被清掉时,那只记录被清掉的成员也一起不再下发通用对端设备类型。
    你这次没有文字回复,我们无法判断这是不是产品上要的行为。
  • 建议:请回一句结论:这种混合情形在产品里是否存在、整组跳过是否为预期。
    若混合情形成立且要保留普通成员的正常下发,按上一轮给的方案 B 改(只跳过记录缺失的成员);只求判断一致则按方案 A 改(统一成组级判据)。
  • 请回复:已修复 / 不改的理由

S3 | test/unittest/services_test/service_test/cdsm_test/nearlink_cdsm_test.cpp:781 —— 新加的用例跑不到整组跳过的那条分支

  • 现象:你补了一个用例(:759-785),方向是对的。但它保护不到要保护的分支:测试里查询厂商类型一律返回 0,而「私有耳机」的类型值是 0x01,所以「组内有一只私有耳机」在用例里永远不成立。
    另外这个用例没有断言,分支即使写反也不会失败;最后一行传进去的地址是普通成员,与真实调用处取的上报地址不同,实际被跳过的恰恰是普通成员。
  • 建议:① 让测试桩按地址返回私有类型,默认仍返回 0,既有用例不受影响;② 补两条断言——混合组(一只私有、一只普通)期望全都不下发,全普通组期望除上报地址外全部下发;③ 调用处改用与真实调用一致的地址,并经由测试类里的公开包装调用,避免从用例体直接访问私有方法。
  • 请回复:已采纳 / 不采纳的理由

已核实通过(2 条)

  • S1 —— 声明、实现体与两处循环四处都加上了 const,与相邻两个函数的写法一致;两个调用处传的都是可写对象,改后绑定不受影响,按改动后的代码逐处核对无遗漏。
  • S2 —— 组内判断改成命中即停,判断结果与逐个查完等价;被查的是设备记录的纯读取,提前停下不会丢任何写入,阻塞次数只减不增。

已结清 / 已关闭(0 条)

  • 本轮无撤回项。

核实方式:你这次没有文字回复,是直接推了新版本,我们按新版本逐条核实——修复项用改动前后逐行对照,再按新版本重读确认;用例项另核了测试桩的返回值与真实调用处的入参。行号一律以复查版本为准。

完整报告含逐条行号依据与两轮证据链,如需索取请回复一句,我们单独发给你。

likedislike
HHo
14 天前 强制推送  1 个提交:2bdb4d51-耳机厂商判断逻辑优化
HHo
14 天前 强制推送  1 个提交:0ff7c16d-耳机厂商判断逻辑优化
HHo
14 天前 强制推送  1 个提交:c2e0596a-耳机厂商判断逻辑优化
HHo
14 天前 强制推送  1 个提交:8c250c4c-耳机厂商判断逻辑优化
HHo
14 天前 强制推送  1 个提交:b5d815b8-耳机厂商判断逻辑优化
HHo
14 天前 强制推送  1 个提交:dd8946de-耳机厂商判断逻辑优化
Ho
13 天前 评论:

检视意见 · PR !319「耳机厂商判断逻辑优化」回复:

  1. W1 -- 一副耳机中,两只耳机不同厂商(私有/非私有)的混合情型在产品中不存在,整组跳过为设计预期。
  2. 已补充tdd用例,混合组(一只私有、一只普通)期望全都不下发,全普通组期望除上报地址外全部下发的场景均覆盖
likedislike
lilong32lilong32成员
13 天前 通过审查
lilong32
lilong32成员
13 天前 评论:

FORCE_VERIFY

likedislike
openharmony_ciopenharmony_ci成员
13 天前 通过测试
openharmony_ci
openharmony_ci成员
13 天前 评论:

添加 测试通过 成功!

likedislike
openharmony_ciopenharmony_ci成员
13 天前 关闭了关联的issue
openharmony_ciopenharmony_ci成员
13 天前 合入了pull request,合并节点 SHA:38574ace9df436081823c7e988b009132b49fff4
openharmony_ciopenharmony_ci成员
13 天前 删除了label:waiting_on_author
openharmony_ciopenharmony_ci成员
13 天前 添加了label:merged