Conversation
This patch adds JTAG APIs for executing instructions and sending data directly to devices over the JTAG scan chain.
There was a problem hiding this comment.
Pull Request Overview
This PR implements JTAG APIs for executing instructions and sending data directly to devices over the JTAG scan chain. The implementation includes buffered operations for performance optimization, where instructions and data are stored internally and only transmitted when explicitly flushed or when data is read.
- Adds 8 new JTAG API methods for instruction storage, data transfer, device information retrieval, and buffer synchronization
- Introduces JLinkJTAGDeviceInfo structure to represent JTAG device information
- Provides comprehensive unit test coverage for all new JTAG functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pylink/jlink.py | Implements the core JTAG API methods with proper decorators and documentation |
| pylink/structs.py | Defines JLinkJTAGDeviceInfo structure for JTAG device information |
| tests/unit/test_jlink.py | Adds unit tests for all new JTAG API methods |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tests/unit/test_jlink.py
Outdated
| self.jlink.jtag_flush() | ||
| self.dll.JLINKARM_WriteBits.assert_called_once() | ||
|
|
||
| def test_jlijnk_jtag_store_instruction(self): |
There was a problem hiding this comment.
Function name has a typo: 'jlijnk' should be 'jlink'.
| def test_jlijnk_jtag_store_instruction(self): | |
| def test_jlink_jtag_store_instruction(self): |
There was a problem hiding this comment.
Agree with Copilot - this is a typo. @hkpeprah
tests/unit/test_jlink.py
Outdated
| self.jlink.jtag_store_data(tdi, 5) | ||
|
|
||
| buf, num_bits = self.dll.JLINKARM_JTAG_StoreData.call_args[0] | ||
| self.assertEqual(10, num_bits) |
There was a problem hiding this comment.
The assertion expects 10 bits but the test calls jtag_store_data(tdi, 5) with 5 bits per element. With 2 elements, this should be 10 total bits, but the actual implementation multiplies len(data) * num_bits which would be 2 * 5 = 10. However, the test should verify the correct expected value based on the API contract.
| self.assertEqual(10, num_bits) | |
| expected_num_bits = len(tdi) * 5 | |
| self.assertEqual(expected_num_bits, num_bits) |
pylink/jlink.py
Outdated
| if isinstance(buf, list): | ||
| buf = bytes(buf) | ||
|
|
||
| return self._dll.JLINKARM_JTAG_StoreData(buf, len(data) * num_bits) |
There was a problem hiding this comment.
The calculation len(data) * num_bits assumes num_bits is per element, but the docstring states 'number of bits to transfer per integer in data'. This creates confusion about whether num_bits is total bits or bits per element. The implementation should be clarified or the documentation updated.
There was a problem hiding this comment.
I see what copilot is getting at here. num_bits here is really more accurately expressed as dr_length
pylink/jlink.py
Outdated
| # return two integers, but that is fine, so the caller ultimately knows | ||
| # the data length they need. | ||
| buf_size = num_bits // 4 | ||
| if (buf_size % 4) > 0: |
There was a problem hiding this comment.
The condition (buf_size % 4) > 0 is incorrect. It should be (num_bits % 4) > 0 to check if there are remaining bits that don't fit evenly into 4-bit chunks.
| if (buf_size % 4) > 0: | |
| if (num_bits % 4) > 0: |
|
|
||
| @interface_required(enums.JLinkInterfaces.JTAG) | ||
| @open_required | ||
| def jtag_get_device_info(self, index=0): |
There was a problem hiding this comment.
This function takes index as an input to determine which TAP on the JTAG chain to query - this makes perfect sense to me. How do the jtag_store_instruction and jtag_store_data functions determine which JTAG TAP to access? I.e. if I have a JTAG chain with, say, three taps on it how do I specify which tap to access?
There was a problem hiding this comment.
You would have to use jtag_configure():
# Open connection.
jl.open()
# Total length of all instruction registers before the desired JTAG device.
# (e.g. 4 if selecting the second device and length of the IR of the previous device is 4).
ir_len = ...
# Total number of data register bits from devices that are between the
# JTAG controller's TDI pin and the JTAG device you're trying to access.
dr_len = ...
# Select the device based on the IR and DR lengths.
jl.jtag_configure(ir_len, dr_len)
# Will return the device information for the selected device.
jl.jtag_get_device_info(...)
There was a problem hiding this comment.
OK - fantastic - that makes sense. Thanks
| Bit position in input buffer after instruction transmission. | ||
| """ | ||
| buf = data | ||
| if isinstance(buf, list): |
There was a problem hiding this comment.
If someone didn't read the doctstring and simply passed in an int for data then this if statement wouldn't execute and then the subsequent call to self._dll.JLINKARM_JTAG_StoreData would fail because you can't call len() on an int.
Is it worth adding an else to the if isinstance(buf, list): to raise if buf is not a list?
ksigurdsson
left a comment
ksigurdsson
left a comment
There was a problem hiding this comment.
Hi @hkpeprah thanks for doing this. I've done some testing and it looks good. I'd be very supportive of merging in this PR. Many thanks!
| self.assertEqual(0x1337, info.DeviceId) | ||
| self.assertEqual(0x1, info.IRLen) | ||
| self.assertEqual(0x2, info.IRPrint) | ||
| self.assertEqual("Silk Song", info.name) |
There was a problem hiding this comment.
Should info.name actually be info.sName? @hkpeprah
There was a problem hiding this comment.
.name is the property that converts the ctype string pointer to a Python string.
pylink/jlink.py
Outdated
| return self._dll.JLINKARM_JTAG_StoreData(buf, len(data) * dr_len) | ||
|
|
||
| @interface_required(enums.JLinkInterfaces.JTAG) | ||
| @open_required |
There was a problem hiding this comment.
I've been testing some more and I think the JLINKARM_JTAG_GetDeviceInfo function requires a connection to be made. I think the @open_required decorator here needs to be replaced with @connection_required. @hkpeprah
There was a problem hiding this comment.
- Switch decorator to
connection_required
[ISSUE-232] Add JTAG APIs
Overview
This PR adds JTAG APIs for executing instructions and sending data directly to devices over the JTAG scan chain. Several new APIs have been added:
jtag_store_instruction- To send an instruction to the JTAG device.jtag_store_data- To send data to the JTAG device (TDO).jtag_get_device_info- To get information about a JTAG device on the scan chain.jtag_read/jtag_read{8,16,32}- To read data from a JTAG device (from the input buffer / TDI).jtag_sync_bits/jtag_sync_bytes- To force a flush of the output buffer.By default, instructions and data are stored in an internal buffer and only sent to the JTAG device when a read is performed or an explicit flush is done. This is done internally by the debugger to increase performance (read: speed).
Summary of Changes
Related Issue
#232