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

AP_Scripting: Adds I2C transfer() bindings and an example #27287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IanBurwell
Copy link
Contributor

Adds LUA bindings for the I2CDevice transfer() function. This is often needed over write_register() as I2C devices might not have 8 bit registers or desire some other custom transfer.

An example script is also added that allows shutdown of a BQ40Z bms chip which would not be possible with LUA without the transfer() function. This script has functionality similar to a sibling PR that implements such functionality more generally in the AP_BattMonitor domain.

@IanBurwell
Copy link
Contributor Author

Sibling PR: #27288

@IanBurwell IanBurwell marked this pull request as draft June 12, 2024 15:09
@IanBurwell IanBurwell force-pushed the scr_transfer branch 3 times, most recently from 5b110f1 to 004b6b3 Compare June 12, 2024 22:12
@IanBurwell IanBurwell marked this pull request as ready for review June 12, 2024 23:31
@IamPete1
Copy link
Member

How do you feel about a string vs the tables? The pack and unpack string functions are very powerful for extracting values with the correct format. Its a shame we have the table precedent in the existing read function.

@IanBurwell
Copy link
Contributor Author

IanBurwell commented Jun 13, 2024

I hadn't considered taking/returning a string which would be more memory efficient, and it would totally be satisfying to be able to do something like

local some_uint32 = string.unpack("<I4", dev:transfer(string.pack("b", 0xAB), 4))

where the string.unpack in handling the byte assembly into a uint32.

But there is also a nice simplicity with using tables. If I wanted just one byte back after a multi byte write it seems like a bit much to do:

local some_byte = string.unpack("b", dev:transfer(string.pack("bbb", 0xAB, 0xCD, 0xEF), 1))

vs

local some_byte = dev:transfer({0xAB, 0xCD, 0xEF}, 1)[1]

But the more I think about this the more a string seems to help even if it incurs a bit more boilerplate. I am going to try and throw together something that uses strings instead.

@IanBurwell
Copy link
Contributor Author

@IamPete1 I have added an alternative transfer_str binding for comparison and an example that I actually used it to debug with. Do you think we should go with one over the other, or keep both (maybe renaming them slightly)?

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have only really reviewed the string version, I don't think we need both, it will just confuse people. Were tending to use strings now for serial and mavlink. I think string is the way to go, but then I have not used either. You have used both, which do you prefer?

libraries/AP_Scripting/examples/BQ40Z_bms_shutdown.lua Outdated Show resolved Hide resolved
libraries/AP_Scripting/examples/RM3100_self_test.lua Outdated Show resolved Hide resolved
libraries/AP_Scripting/lua_bindings.cpp Outdated Show resolved Hide resolved
libraries/AP_Scripting/lua_bindings.cpp Outdated Show resolved Hide resolved
libraries/AP_Scripting/lua_bindings.cpp Outdated Show resolved Hide resolved
libraries/AP_Scripting/lua_bindings.cpp Outdated Show resolved Hide resolved
libraries/AP_Scripting/lua_bindings.cpp Outdated Show resolved Hide resolved
@IanBurwell
Copy link
Contributor Author

IanBurwell commented Jul 11, 2024

Having used them both, I do now prefer the string version even though it brings some extra boilerplate with it. string.unpack is just too powerful.

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

Successfully merging this pull request may close these issues.

None yet

2 participants