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

[Fix] otaclient_stub: fix always export self ECU id in available_ecu_ids #250

Merged
merged 5 commits into from
Aug 3, 2023

Conversation

Bodong-Yang
Copy link
Member

@Bodong-Yang Bodong-Yang commented Aug 2, 2023

Description

Note
This PR superseded #249 .

This PR fixes unexpected behavior that otaclient_stub.ECUStatusStorage always add self ECU id into available_ecu_ids field when exporting. The bug is introduced when otaclient_stub is fully refactored in #212 .

Concepts related to ECUs for loop status querying

otaclient_stub keeps track of all reachable ECUs' status by loop polling the status API of all directly connected sub ECUs and query self ECU's internal status API. It categorizes ECUs into three groups as follow:

  1. available_ecu_ids: a field that defined in ecu_info.yaml and merging from sub ECUs' status's available_ecu_ids field, and exported in status API response. This serves information purpose only and should be passed to upper agent as it. For web.auto agent and being used to generate update request,
  2. tracked_ecus(active_ota_ecus): ECUs that is listed in the update request and actively doing OTA update,
  3. all_reachable_ecus: ECUs that are reachable and status response can be seen, including all ECUs defined in ecu_info.yaml(self ECU and all directly connected sub ECUs), and all further child ECUs.

Cause of the bug

Previously otaclient_stub.ECUStatusStorage's implementation internally mixes the concepts of available_ecu_ids, tracked_ecus(active_ota_ecus) and all_reachable_ecus into single attribute, effectively making ECUStatusStorage only uses all_reachable_ecus internally and mistakenly export this value in the status API response as available_ecu_ids.

Bug fix

Current behavior

  1. self ECU id is always being included in the available_ecu_ids, even it is not in ecu_info.yaml.

Behaivor after fix

  1. available_ecu_ids exported in status API response should be the same as ecu_info.yaml and merging from all sub ECUs' available_ecu_ids,
  2. for ECUStatusStorage, available_ecu_ids, active_ota_ecus and all_reachable_ecus are clearly distinguished internally:
    a. available_ecu_ids will only be updated by ecu_info.yaml and merging from sub ECUs' status report,
    b. overall_ecu_status_report is only generated against active_ota_ecus(besides lost_ecus, this property is still generated against all_reachable_ecus),
    c. export API will always export all status stored(status of all_reachable_ecus), except for ECUs that become unreachable or disconnected for a while.

Related links & ticket

  1. [Feature&refinement] implement new multiple ECU OTA update spec, refactoring otaclient_stub module #212
  2. doc: update document especially available_ecu_ids #248
  3. OTA service API handler design document: document is updated to include new information related to available_ecu_ids.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

Coverage

Coverage Report
FileStmtsMissCoverMissing
otaclient/app
   __init__.py00100% 
   __main__.py330%16–19
   common.py2681594%55, 103, 185, 200, 275–277, 287, 296–298, 404, 416, 535–539
   configs.py710100% 
   copy_tree.py81396%45, 96, 125
   downloader.py2704484%69, 82–83, 300, 305, 309, 327–328, 378–382, 401–411, 436–454, 463, 538–540, 556, 576–594
   ecu_info.py59788%78–79, 92, 97, 120–121, 130
   errors.py179597%55, 58, 116, 119, 139
   interface.py17476%32, 43, 47, 51
   log_setting.py26773%27–28, 55–65
   main.py31294%43–44
   ota_client.py3188773%69–70, 92, 198–222, 259–261, 271–276, 286–288, 331–332, 337–344, 357–371, 377–380, 405–429, 443–448, 457, 460–468, 547, 552–570
   ota_client_call.py38684%42–44, 80–82
   ota_client_service.py29390%58–60
   ota_client_stub.py3457578%81–90, 99–101, 106, 138, 162, 165, 168, 272, 298, 300, 326, 376, 438, 508–509, 548, 568–579, 583–597, 601–604, 656, 743–752, 784–834, 837
   ota_metadata.py3143190%145, 150, 186–187, 197–201, 213, 271, 304–306, 323–326, 406, 409, 417–419, 432, 441–446, 719–730
   ota_status.py15287%36, 44
   proxy_info.py51786%82–87, 122–129, 137–138
   update_stats.py105298%162, 172
otaclient/app/boot_control
   __init__.py40100% 
   _cboot.py28613951%91–99, 108–121, 127–130, 134, 141, 145, 149, 153–157, 161–165, 170–189, 192–235, 241, 244, 247, 250, 253, 256, 259, 262–263, 266–267, 270–274, 277–278, 293, 296, 329, 356–359, 369–372, 380, 419, 436–439, 448–451, 456–458, 467–473, 476, 483, 513–515, 552–554, 557–567, 570–575
   _common.py36315557%65–74, 83–84, 88–89, 97–98, 109–110, 115–120, 125–130, 139, 148, 152, 156, 160, 174–177, 187–191, 196, 200, 210–214, 222–234, 245–249, 253–257, 269–271, 283–285, 296–310, 320–341, 360–376, 393–404, 412–418, 467, 489–496, 518–539, 577–578, 632, 636–637, 640–648, 652–655, 705–706, 709, 723–731, 746–747, 816–819, 840–843, 856–857, 866
   _errors.py471960%43–69, 89–90
   _grub.py4029078%202, 224, 241–244, 250–254, 280–281, 288–308, 318–320, 328–330, 334, 337, 340, 343, 365–366, 384–385, 433, 468, 488–493, 506, 533–542, 679–681, 708–711, 720, 759–763, 807, 814, 833–836, 843–846, 849, 856, 883–885, 906–908, 911–916, 919–924
   _rpi_boot.py26012153%89–91, 97–152, 176–178, 184–186, 199–201, 207–209, 222–243, 248, 252, 256, 260, 294, 324, 327–329, 337, 346–348, 358–361, 365–372, 451–455, 474–477, 482, 485, 506–511, 514–522, 525–533, 545–548, 552–554, 557
   configs.py57296%46–47
   firmware.py32584%63, 65, 76–78
   protocol.py28582%40, 44, 48, 52, 60
   selecter.py392731%45–69, 77–94
otaclient/app/create_standby
   __init__.py12558%30–36
   common.py2164579%74, 77–78, 82–95, 141, 189–197, 200–203, 207, 218, 292–300, 312, 356–361, 377–378, 392–396, 421–422
   interface.py19384%55, 59, 63
   rebuild_mode.py99991%83–101, 128
otaclient/app/proto
   __init__.py31390%37, 44–45
   _common.py4194988%85, 163, 170, 182–184, 203, 208, 219, 256, 262, 267, 298, 302, 306, 363, 380, 403, 464, 471, 474, 494, 501, 503, 529, 535, 538, 540, 565, 571, 574, 576, 605, 609, 611, 625, 642, 670, 673, 677, 680, 708, 714, 761–766, 797
   _ota_metafiles_wrapper.py841385%37, 40–42, 112–116, 122–125
   _otaclient_v2_pb2_wrapper.py2763687%87, 90–93, 175, 183, 197, 207, 210–213, 258, 261, 264–265, 285, 305, 385, 452, 505, 513–515, 519–522, 525–530, 551, 559, 573, 581, 595
   streamer.py43881%32, 47, 65–66, 71, 80–81, 99
   wrapper.py40100% 
otaclient/ota_proxy
   __init__.py291066%53–58, 102–123
   __main__.py21210%16–71
   _consts.py150100% 
   cache_control.py84890%50, 75, 95, 117, 125, 141, 147–148
   config.py180100% 
   db.py1461590%75, 81, 103, 113–116, 145–147, 166, 199, 208–209, 229, 258, 300
   errors.py100100% 
   orm.py1151190%37, 91, 96, 101, 107, 113, 140–141, 154, 231, 235
   ota_cache.py3897980%91–92, 94–95, 214, 225, 252–254, 274, 290–293, 316–329, 358–362, 378, 439–440, 482–483, 553, 566–569, 633–634, 666–667, 678, 712–750, 797, 805–807, 878–881, 885–889, 924, 930, 971–973
   server_app.py1383972%75, 78, 84, 100, 102, 161–170, 212–243, 256–266, 292–298, 312–314, 320–322
   utils.py23196%31
TOTAL5929122179% 

@Bodong-Yang Bodong-Yang self-assigned this Aug 2, 2023
@Bodong-Yang Bodong-Yang added the bug Something isn't working label Aug 2, 2023
@Bodong-Yang Bodong-Yang requested a review from obi-t4 August 2, 2023 04:43
@Bodong-Yang Bodong-Yang marked this pull request as ready for review August 2, 2023 04:43
@obi-t4
Copy link
Contributor

obi-t4 commented Aug 2, 2023

@Bodong-Yang

available_ecu_ids: a field that defined in ecu_info.yaml and merging from sub ECUs' status's available_ecu_ids field, and exported in status API response.

sub ecu doesn't have this available_ecu_ids.
See https://github.com/tier4/ota-client/blob/main/docs/README.md?plain=1#L148

@obi-t4
Copy link
Contributor

obi-t4 commented Aug 2, 2023

available_ecu_ids: a field that defined in ecu_info.yaml and merging from sub ECUs' status's available_ecu_ids field, and exported in status API response.

Ah, you're saying status API response, and sub ECU's statu's available_ecu_ids always empty, right?

@Bodong-Yang
Copy link
Member Author

Bodong-Yang commented Aug 3, 2023

@Bodong-Yang

available_ecu_ids: a field that defined in ecu_info.yaml and merging from sub ECUs' status's available_ecu_ids field, and exported in status API response.

sub ecu doesn't have this available_ecu_ids. See https://github.com/tier4/ota-client/blob/main/docs/README.md?plain=1#L148

That is not a big problem, if sub ECU doesn't have available_ecu_ids, then the merging just merges empty list, which doesn't impact anything.
In contrast, this behavior of otaclient_stub can be treated as forward-compatibility to multi-layer structure. If in the future we support multi-layer structure, and sub ECUs also have their own child ECUs, we can support it without any changes to the code.

Copy link
Contributor

@obi-t4 obi-t4 left a comment

Choose a reason for hiding this comment

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

It is working as expected!

@@ -408,7 +444,6 @@ async def update_from_local_ecu(self, ecu_status: wrapper.StatusResponseEcuV2):
"""Update ECU status storage with local ECU's status report(StatusResponseEcuV2)."""
async with self._writer_lock:
self.storage_last_updated_timestamp = cur_timestamp = int(time.time())
self._all_available_ecus_id.add(ecu_status.ecu_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Bodong-Yang Bodong-Yang merged commit e7f4b31 into main Aug 3, 2023
3 checks passed
@Bodong-Yang Bodong-Yang deleted the fix/available_ecu_ids branch August 3, 2023 07:30
Bodong-Yang added a commit that referenced this pull request Aug 30, 2023
Commits from v3.5.1 to v3.6.0

* Bug fix
1. [Fix] boot_ctrl.cboot: post_update error handling #240
2. [Fix] otaclient_stub: fix always export self ECU id in available_ecu_ids  #250

* Features
1. [Feature] extend ota-file-cache-control header #244
2. [Feature & Refinement] otaproxy: OTA file sha256 aware, support new header  #245
3. [Feature & Refinement] otaproxy, otaclient: introduce external cache dev support #246

* Chore
1. doc: update document especially available_ecu_ids #248
2. [Chore] tools: add new OTA status monitor #251
3. [Chore] tools: add offline OTA image builder #252

* Deps
1. Bump grpcio from 1.40.0 to 1.53.0 in /tools/emulator #241
2. [Deps] bumps grpcio to 1.53.1 #243
3. Bump aiohttp from 3.8.1 to 3.8.5 in /otaclient #247
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants