Skip to content
New issue

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

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

Already on GitHub? # to your account

Add switch_map method to connect_openrmf_by_zenoh.py #12

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

h-wata
Copy link
Contributor

@h-wata h-wata commented Jun 13, 2024

Summary

Detail

Impact

Test

Attention

@h-wata h-wata self-assigned this Jun 13, 2024
@h-wata h-wata marked this pull request as draft June 13, 2024 07:36
@github-actions github-actions bot added the enhancement New feature or request label Jun 13, 2024
Copy link

Title

Add switch_map method to connect_openrmf_by_zenoh.py


PR Type

Enhancement


Description

  • switch_mapメソッドをconnect_openrmf_by_zenoh.pyに追加し、指定されたマップにロボットを切り替える機能を実装。
  • config.yamlmethod_mappingchange_mapを追加し、switch_mapメソッドとマッピング。
  • _command_callbackswitch_mapメソッドを呼び出すロジックを追加。

Changes walkthrough 📝

Relevant files
Enhancement
connect_openrmf_by_zenoh.py
`switch_map`メソッドの追加とコールバックでの呼び出し                                                 

scripts/connect_openrmf_by_zenoh.py

  • switch_mapメソッドを追加
  • switch_mapメソッドの引数として辞書を受け取る
  • switch_mapメソッドを_command_callbackで呼び出す
  • +13/-0   
    Configuration changes
    config.yaml
    `method_mapping`に`change_map`を追加                                                 

    config/config.yaml

    • method_mappingchange_mapを追加
    +1/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    2

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The switch_map method does not handle the case where map_id is None. This could lead to an unhandled exception.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Performance
    asyncio.runの呼び出しを一度にまとめます。


    _command_callbackメソッド内でasyncio.runを複数回呼び出すのではなく、一度の呼び出しで済むようにリファクタリングすることで、パフォーマンスの向上とコードの簡潔化を図ることができます。

    scripts/connect_openrmf_by_zenoh.py [180-182]

     if method_name == "switch_map":
    -    asyncio.run(self.switch_map(command['args']))
    -asyncio.run(self.run_method(method_name, command['args']))
    +    await self.switch_map(command['args'])
    +await self.run_method(method_name, command['args'])
     
    Suggestion importance[1-10]: 10

    Why: asyncio.runの呼び出しを一度にまとめることで、パフォーマンスが向上し、コードが簡潔になります。これは重要なパフォーマンス改善です。

    10
    Possible issue
    map_idが見つからない場合のエラーハンドリングを追加します。


    switch_mapメソッド内でmap_idが見つからない場合のエラーハンドリングを追加することを検討してください。これにより、map_idが見つからない場合に適切なエラーメッセージを表示することができます。

    scripts/connect_openrmf_by_zenoh.py [125-128]

     if map_id is not None:
         payload = {"map_id": map_id, "pose": args.get("pose", {"x": 0.0, "y": 0.0, "theta": 0.0})}
         await self.run_method("switch_map", payload)
    +else:
    +    print(f"Error: Map name '{args.get('map_name')}' not found.")
     
    Suggestion importance[1-10]: 9

    Why: エラーハンドリングを追加することで、ユーザーに対して明確なフィードバックを提供し、デバッグを容易にすることができます。これは重要な改善です。

    9
    Best practice
    next関数のデフォルト値を明示的にNoneに設定します。


    next関数を使用する際に、デフォルト値を指定することで、map_idが見つからない場合にNoneを返すようにするのではなく、Noneをデフォルト値として明示的に指定する方が良いです。これにより、map_idが見つからない場合の挙動が明確になります。

    scripts/connect_openrmf_by_zenoh.py [124]

    -map_id = next((item["id"] for item in map_list if item["name"] == args.get('map_name')))
    +map_id = next((item["id"] for item in map_list if item["name"] == args.get('map_name')), None)
     
    Suggestion importance[1-10]: 8

    Why: 明示的にデフォルト値を指定することで、コードの意図が明確になり、予期しないエラーを防ぐことができます。これはベストプラクティスに従った改善です。

    8
    Maintainability
    デフォルトのpose値を定数として定義します。


    switch_mapメソッド内のargs.getのデフォルト値として使用されるposeの値を定数として定義し、再利用可能にすることで、コードの可読性と保守性を向上させることができます。

    scripts/connect_openrmf_by_zenoh.py [126]

    -payload = {"map_id": map_id, "pose": args.get("pose", {"x": 0.0, "y": 0.0, "theta": 0.0})}
    +DEFAULT_POSE = {"x": 0.0, "y": 0.0, "theta": 0.0}
    +payload = {"map_id": map_id, "pose": args.get("pose", DEFAULT_POSE)}
     
    Suggestion importance[1-10]: 7

    Why: デフォルト値を定数として定義することで、コードの可読性と保守性が向上しますが、これは主にコードスタイルの改善であり、機能に大きな影響はありません。

    7

    @h-wata h-wata force-pushed the feature/switch_map branch from 4054ea1 to 59bce54 Compare August 2, 2024 06:44
    @h-wata h-wata force-pushed the feature/switch_map branch from 59bce54 to 7018cf6 Compare January 8, 2025 06:06
    # for free to join this conversation on GitHub. Already have an account? # to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    地図の切り替え機能
    1 participant