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

Add new Wire.sendTo(...) and Wire.requestFrom(...) API's that use external buffer #269

Conversation

sandeepmistry
Copy link
Contributor

@sandeepmistry sandeepmistry commented Oct 16, 2017

Proposal to add new Wire lib API's that allow the caller to specify an external buffer to use. Use cases:

  • to avoid extra overheads of using Wire's internal buffer + Stream API's
  • when the size of Wire's internal buffer is not large enough

New APIs:

uint8_t sendTo(uint8_t address, uint8_t buffer[], size_t quantity);
uint8_t sendTo(uint8_t address, uint8_t buffer[], size_t quantity, bool stopBit);

uint8_t requestFrom(uint8_t address, uint8_t buffer[], size_t quantity);
uint8_t requestFrom(uint8_t address, uint8_t buffer[], size_t quantity, bool stopBit);

@arduino arduino deleted a comment from ArduinoBot Nov 22, 2017
@ArduinoBot
Copy link

✅ Build completed.

⬇️ Build URL: http://downloads.arduino.cc/PR/samd/package_samd-b179_index.json

ℹ️ To test this build:

  1. Open the Preferences of the Arduino IDE.
  2. Add the Build URL above in the Additional Boards Manager URLs field, and click OK.
  3. Open the Boards Manager (menu Tools->Board->Board Manager...)
  4. Install Arduino SAMD core - Pull Request Add new Wire.sendTo(...) and Wire.requestFrom(...) API's that use external buffer #269
  5. Select one of the boards under SAMD Pull Request Add new Wire.sendTo(...) and Wire.requestFrom(...) API's that use external buffer #269 in Tools->Board menu
  6. Compile/Upload as usual

@arduino arduino deleted a comment from ArduinoBot Nov 22, 2017
@lupalby
Copy link

lupalby commented Dec 18, 2017

The idea is great, I think the library should definitely allow the user to provide an existing buffer to be sent or filled, so that that there is no hard-coded limit on its size (#285).
Your idea of expanding the requestFrom function is definitely the best way I can think of.
The only problem I see in this implementation is that now there would be 2 complete different ways to send data through the wire library. One is using beginTransmission+write+endTrasnmission, and the other one is just by using sendTo. I personally never liked the former, but that's what everybody have been using in their sketch since forever and it may be confusing to have these 2 different ways both officially supported.

Why don't you expand the functionality of the write function, so that it can accept the buffer as argument? Then it would maintain the beginTransmission+write+endTrasnmission approach, but if I call write() passing a pointer+size instead of a char it would set the library to use the buffer I give rather than the internal ring buffer. Then endTransmission would send the data using the ring buffer or, if set, my buffer. beginTrasmission would always default the buffer to be the ring buffer, for compatibility.

What do you think? It may be a bit more consistent with the current API, but still giving the user the possibility to provide a buffer.

@lupalby
Copy link

lupalby commented Dec 18, 2017

Alternatively you can just extend beginTransmission to also take a buffer as input, and that would set the library to use that buffer for all subsequent operations (write+endTransmission) instead of the ring buffer. It is another option but I don't like it as much because if I already have all data sitting in an array somewhere, that would still require to copy it all byte by byte with many write() calls. I personally prefer the approach of my previous comment (#269 (comment)) because if I have some other libraries serializing the data for me, then I can simply provide a pointer to that array to the Wire lib and be done with it, without extra duplications

@sandeepmistry
Copy link
Contributor Author

@lupalby good points.

Could you please share the method signatures along with some sketch snippets you have in mind just to clarify?

@lupalby
Copy link

lupalby commented Dec 19, 2017

I think I was a little confusing when talking about how to extend the functions. Here a more detail explanation.

You could add 2 private global variables
uint8_t* externalBuffer = NULL;
size_t externalBufferSize = 0;

beginTransmission would always set externalBuffer = NULL as default, and flush the ring buffer ( just to be safe).
If people use Wire.write() in any of the forms present now, data will be added to the ring buffer as it is done right now, for compatibility.

Write could be expanded like this:
size_t TwoWire::write(const uint8_t *data, size_t quantity, bool useProvidedBufferDirectly)
This variation of the write function would set externalBuffer=data and externalBufferSize=quantity when the bool is true, and it would be the regular write() when the bool is false (uses the ring buffer).

So, for compatibility
size_t TwoWire::write(const uint8_t *data, size_t quantity)
{
return write(data, quantity, false);
}

The implementation of endTransmission can be similar to what you have now in sendTo. You check if externalBuffer==NULL, if yes you use the ring buffer, otherwise you use the buffer that the user provided.

In the sketch it would look like:
char test[]= "This is a buffer provided by the user";
Wire.beginTransmission(SLAVE_ADDRESS);
Wire.write(test, sizeof(test), true);
Wire.endTransmission;

@sandeepmistry
Copy link
Contributor Author

@lupalby how about:

int beginTransmission(uint8_t address, uint8_t* externalBuffer = NULL, size_t externalBufferSize = 0, size_t prepopulatedSize = 0);

@lupalby
Copy link

lupalby commented Dec 20, 2017

@sandeepmistry What would be the advantage? Probably that you can still use Wire.write passing few bytes at a time, but it would write on the provided buffer instead of the ring one. Is there any other reason?
I can see your point, it may make more sense, it would be even easier to adapt old code to work with the new system

@sandeepmistry
Copy link
Contributor Author

@lupalby Advantages I can think of:

  • you can pre-populate some or all of the data in the buffer
  • continue to use the nice Stream and Print API's to fill the remainder of the buffer
  • if you need a bigger buffer you can supply it

@lupalby
Copy link

lupalby commented Dec 22, 2017

@sandeepmistry It makes sense. Are you gonna update the code of this pull request with what we discussed? I'll be happy to review it once you commit

@sandeepmistry
Copy link
Contributor Author

Hi @lupalby,

That would be great! I opened a new pull request for these changes: #289.

@sandeepmistry
Copy link
Contributor Author

Closing in favour of #298.

boseji pushed a commit to go-ut/combined-ArduinoCore-samd that referenced this pull request Nov 23, 2020
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

4 participants