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

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

Open
AntonSalland opened this issue May 12, 2023 · 5 comments
Open

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

AntonSalland opened this issue May 12, 2023 · 5 comments

Comments

@AntonSalland
Copy link

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
Copy link
Contributor

hkpeprah commented May 15, 2023

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
Copy link
Author

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
Copy link
Contributor

hkpeprah commented May 23, 2023

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
Copy link
Author

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants