-
Notifications
You must be signed in to change notification settings - Fork 8
Ingk 1193 create table object api #667
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
Conversation
4b8f9f4 to
804250b
Compare
5aadf99 to
a665f9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a table object API for both virtual and real drives, enabling indexed access to table data stored in drive memory. The implementation provides a high-level API for reading and writing table values through index/value register pairs.
Key changes:
- Added
VTableclass for virtual drive table simulation with signal-based index/value access - Implemented
TableAPI class providing bracket notation, iteration, and bulk read/write operations - Extended dictionary parsing to support table definitions with axis specifications
- Added comprehensive test coverage for both virtual and real servo scenarios
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| virtual_drive/resources/table.py | New VTable class for virtual drive table implementation with signal watchers |
| virtual_drive/core.py | Integrated user memory table (256 depth) with signal registration |
| ingenialink/table.py | New Table API class with read/write, iteration, and bracket notation support |
| ingenialink/dictionary.py | Added axis support to DictionaryTable, implemented get_table method and table merging |
| ingenialink/servo.py | Added get_table method to retrieve Table objects from dictionary |
| tests/test_table.py | Comprehensive test suite for both virtual and real servos with table operations |
| tests/test_dictionary.py | Tests for table parsing, retrieval, and dictionary merging |
| tests/resources/dictionary_with_tables_minimal.xdf3 | Updated table definitions to include axis attribute |
| tests/resources/init.py | Renamed constant and added new dictionary resource reference |
| pyproject.toml | Updated summit-testing-framework dependency version |
| poetry.lock | Updated lock file for new dependency version |
| ingenialink/utils/_utils.py | Refactored type alias for register values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| axis = int(_axis) if _axis is not None else 0 | ||
| id_index = str(table_element.attrib.get(self.__TABLE_ID_INDEX_ATTR)) | ||
| id_value = str(table_element.attrib.get(self.__TABLE_ID_VALUE_ATTR)) | ||
|
|
||
| table = DictionaryTable( | ||
| id=str(uid), | ||
| id_index=str(id_index), | ||
| id_value=str(id_value), | ||
| id=uid, | ||
| axis=axis if _axis is not None else None, |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistency in axis handling: When _axis is None, the code sets axis = 0 for storage purposes (line 1614) but then sets axis = None in the DictionaryTable object (line 1620). This creates a mismatch between the storage location (axis 0) and the table's actual axis value (None). The logic should consistently use either 0 or None for non-axis-specific tables. Based on the test expectations, tables without an axis attribute should be stored at axis 0 but have axis=None in their DictionaryTable object, which is what the current code does. However, this could be confusing for future maintainers.
| KeyError: if the specified axis does not exist. | ||
| KeyError: if the table is not present in the specified axis. | ||
| ValueError: if the table is not found in any axis, if axis is not provided. | ||
| ValueError: if the table is found in multiple axis, if axis is provided. |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error description is misleading: "if the table is found in multiple axis, if axis is provided" should be "if the table is found in multiple axes, if axis is not provided". When axis IS provided, the method returns the table from that specific axis or raises a KeyError. The ValueError for multiple matches only occurs when axis is None.
| ValueError: if the table is found in multiple axis, if axis is provided. | |
| ValueError: if the table is found in multiple axes, if axis is not provided. |
…3-1' into INGK-1193-create-table-object-api
dd53fe3 to
0fb1c21
Compare
…3-1' into INGK-1193-create-table-object-api # Conflicts: # poetry.lock # pyproject.toml
d72c7ca to
2955352
Compare
| if not isinstance(min_index, int) or not isinstance(max_index, int): | ||
| raise ValueError("Index register must have integer range.") | ||
|
|
||
| if min_index < 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a possibility?
A non-checked max_index out of bounds that makes anything (or some things) to crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually min index is -1. This value is used to avoid write in the table.
…3-create-table-object-api # Conflicts: # poetry.lock
Description
Created table object with useful api to interact with elements of the table.
The previous element DictionaryTable is only used for parsing. With this new element you can boot up the API that also contains the servo object. This will be used in the future for loading and saving xcfs, as well as potentially showing table editing on motionlab.
As explained the virtual drive meeting, I had to put a full fledged dictionary instead of minimal to be able to do tests with virtual drive, this is something that we can improve on the future.
Type of change
Please add a description and delete options that are not relevant.
Tests
Please describe the manual tests that you ran to verify your changes if it applies.
Documentation
Please update the documentation.
[Unreleased]section of the CHANGELOG.