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

import_attribute_configuration_buffer and export_attribute_configuration_buffer should use bytes #1013

Closed
marcoskirsch opened this issue Aug 7, 2019 · 4 comments · Fixed by #1262

Comments

@marcoskirsch
Copy link
Member

#Description of issue

The API for import_attribute_configuration_buffer and export_attribute_configuration_buffer in several nimi-python APIs uses list of int to represent the buffer. This is inefficient and sematically incorrect.

It should use bytes in Python 3 and arguably str in Python 2.

@marcoskirsch
Copy link
Member Author

I tagged this as priority-critical because it's possibly a source breaker. But we may be able to do it in a way in which it isn't or it very rarely is because of duck typing.

@sbethur
Copy link
Contributor

sbethur commented Sep 25, 2019

If we use bytes type, it would result in a data-copy in export_attribute_configuration_buffer since bytes is immutable. We will have to allocate a temp buffer, get export_attribute_configuration_buffer to fill it with data and copy the data to bytes object.

Instead I'm planning to use bytearray, which is a mutable block of memory. It was added to Python in 2.6, so it'll work for all versions we support.

The changes are pretty straightforward - I need to update get_ctypes_pointer_for_buffer() to create a ctypes array object backed by bytearray object's memory:
return (library_type * len(value)).from_buffer(value)
and update export_attribute_configuration_buffer to create a bytearray object.

I also considered using array.array. But there isn't a "bytes" type that can be put into an array and it doesn't seem to offer any benefits for this use-case.

Given users would likely use export/import by saving and restoring from file. I checked if data can be read from a file into an object of desired type without an extra-copy. Couldn't find a way to avoid the extra-copy with either array.array or bytearray. But I didn't dig too deep, since the amount of data we're dealing with is pretty small. I would have to worry about it when I start looking into HRAM streaming in Digital.

Now I need to figure out the changes required in code generator to make the above changes happen.

@marcoskirsch
Copy link
Member Author

From the API point of view, it makes more sense for export_attribute_configuration_buffer to return the immutable bytes type since I don't think there is any reasonable use-case for the customer to mess with the returned data.

I'm not too concerned about the extra data copy because:

  1. export_attribute_configuration_buffer is not a performance sensitive function
  2. the size of the buffers are in the low KB, should be negligible

This function is IMO going to be rarely used. Most customers will use InstrumentStudio to create their configuration as a file and the programmatically import it using import_attribute_configuration_file.

@anandjain-ni
Copy link

Changing the datatype for 'import_attribute_configuration_buffer' will break the interface between NI InstrumentStudio and NI TestStand. I would like for 'import_attribute_configuration_buffer' to still work with lists of int after this change.

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