square / pylink

Python Library for device debugging/programming via J-Link
https://pylink.readthedocs.io/en/latest/
Other
334 stars 125 forks source link

connect("nrf52") raises "ValueError: Invalid index." #174

Open AntonSalland opened 1 year ago

AntonSalland commented 1 year ago

I am trying it connect to an nRF52 device, this can normally be done without specifying the specific chip version in Jlink.

When using pylink it error's out because the index of the device when using get_device_index("nrf52") is 9351 while num_supported_devices() returns 9211.

When connect("nrf52") gets called the index gets checked in the supported_device(index) function (jlink.py line 672)

        if not util.is_natural(index) or index >= self.num_supported_devices():
            raise ValueError('Invalid index.')

When I remove the or index >= self.num_supported_devices() part of that check the device programs as expected. At that point I can iterate through supported_device(index) as far up as index 9542. After witch _dll.JLINKARM_DEVICE_GetInfo(index, info) starts returning non zero (returned value: 9211).

From what I gather this might actually be an issue with the DLL. But unless there is a specific reason the num_supported_devices() check is done, I think a valid work around would be to just validate the result of the _dll.JLINKARM_DEVICE_GetInfo(index, info) call instead of checking the index against num_supported_devices().

OS: win10 DLL version: 7.88b pylink version: 1.1.0

To reproduce:

import pylink
jlink = pylink.JLink()
jlink.open()
jlink.connect("nrf52")
print("success")
jlink.close()

Expected result: success Actual behavior: ValueError: Invalid index.

Suggested workaround: (jlink.py line 659)

    def supported_device(self, index=0):
        """Gets the device at the given ``index``.

        Args:
          self (JLink): the ``JLink`` instance
          index (int): the index of the device whose information to get

        Returns:
          A ``JLinkDeviceInfo`` describing the requested device.

        Raises:
          ValueError: if index is less than 0 or >= supported device count.
        """
        if not util.is_natural(index):
            raise ValueError('Invalid index.')

        info = structs.JLinkDeviceInfo()

        result = self._dll.JLINKARM_DEVICE_GetInfo(index, ctypes.byref(info))

        if not result == 0:
          raise ValueError('Invalid index.')

        return info
hkpeprah commented 1 year ago

That looks fine to me. There's no particular reason we check against the length of the list AFAIK. A patch is more than welcome if you want to author a change!

AntonSalland commented 1 year ago

I'll try to submit a patch some time soon. Still slightly new to git(hub) as well as Python so I will need to figure out how to do that properly first. This seems like a good, relatively simple issue to start with and learn though. Please bare with me.

Should the return value of num_supported_devices() be considered a bug as well? If so this seems slightly less straight forward to fix outside of just iterating through the list. That seems like it would be really inefficient, maybe iterate starting at the return value? It seems num_supported_devices isn't used anywhere else in the module itself. Some guidance on this would be appreciated, otherwise I'll just leave it out of the patch for now.

Should I be updating the unit tests, change log, and list of contributors as well when submitting a patch?

I also found found a related issue (#54). once the patch is done I think this could be closed as well.

hkpeprah commented 1 year ago

Yes, please update the unit tests. I will update the CHANGELOG and CONTRIBUTORS once your patch is merged in, and I cut a new release. I don't think the return value is a bug; not sure we can iterate starting at the return value either since we wouldn't know when to stop?

AntonSalland commented 1 year ago

The reason it might be considered a bug is because if you were to use num_supported_devices to list all the supported devices for instance, you are currently missing out about 300 devices at the end of the list.

To see where to stop we could again check if the return value of _dll.JLINKARM_DEVICE_GetInfo() is unequal to 0 for the given index.

example solution: (untested, might need an extra "index -= 1" at the end if we want to return max index instead of the number of devices.)

    def num_supported_devices(self):
        """comment...
        """
        index = int(self._dll.JLINKARM_DEVICE_GetInfo(-1, 0))

        while (self._dll.JLINKARM_DEVICE_GetInfo(index, 0) == 0):
          index += 1

        return index

current implementation for reference:

    def num_supported_devices(self):
        """comment...
        """
        return int(self._dll.JLINKARM_DEVICE_GetInfo(-1, 0))
hkpeprah commented 1 year ago

Ah okay. If -1 works, then that sounds good to me.