Polishing the library

[BrainStone]

As of right now this a is work in progress though I believe you might want to watch my progress.

TODO:

• Restructure the Packet class
• Prepare the Packet class for inheritance (make it abstract)
• Add back hooks/callbacks
• Reimplement SerialTransfer class (will also get an alias StreamTransfer to indicate that it can work with any type of Stream)
• Reimplement SPITransfer
• Reimplement I2CTransfer
• Optimize PacketCRC
• Document every class and method with doxygen style comments
  • Packet
  • PacketCRC
  • SerialTransfer
  • SPITransfer
  • IC2Transfer
• Update examples

[BrainStone]

Now ignoring documentation this PR is finished.
So feel free to start testing and giving feedback.

One thing that could make sense to do is updating the examples. Do you want them updated in this PR or should I make a separate PR for that?

[BrainStone]

Ok. I take it back. I found a way to make the CRC array compile time constant.
So it's not fully done code wise. But everything else should be for the most part!

[BrainStone]

Here's a little library I drafted up for that purpose: https://github.com/BrainStone/CppCompiletimeArrayGenerator

What's your take on this?
Should I just add the header to the library or make the struct a private struct of PacketCRC to hide the implementation completely?

[PowerBroker2]

If it's only going to be applied to PacketCRC and no other sub-lib, let's just go with the private struct option.

Also, how do I test your code without merging? I thought you were doing the work on your fork, but I don't think that's the case.

Lastly, there are a lot of changes across the lib files so I can't easily tell: did the sketch-level API(s) change, thus requiring example sketch updates like you say or did you simply want to reformat them? I'd prefer to keep the existing API the same (adding to it is fine, however)

[PowerBroker2]

Also, sorry about the squash thing - I'm not a professional software developer. Thank you for the insights, I'm learning a lot!

[BrainStone]

If it's only going to be applied to PacketCRC and no other sub-lib, let's just go with the private struct option.

As far as it seems it's only needed in PacketCRC. While I think that little lib is useful I don't think it belongs here. So a private struct and copying the code over seems to make the most sense

Also, how do I test your code without merging? I thought you were doing the work on your fork, but I don't think that's the case.

I am actually maintaing my own fork. See the line under the PR title: https://github.com/BrainStone/SerialTransfer/tree/inheritance
Alternatively I think you can check it out as a branch. Have a look here: https://docs.github.com/en/enterprise/2.15/user/articles/checking-out-pull-requests-locally

Lastly, there are a lot of changes across the lib files so I can't easily tell: did the sketch-level API(s) change, thus requiring example sketch updates like you say or did you simply want to reformat them? I'd prefer to keep the existing API the same (adding to it is fine, however)

Well it's mostly the same. I did some cleanups here and there, some types changed, some renaming for consitency.
Most notibly constructing the objects has changed. All you need to supply now is handled with the constructors.

So while the examples might need some minor adjustments, nothing major has changed. Like the way the lib works hasn't changed. Just some names
I tried to keep the changes limited to a point where it wouldn't break existing code, but that was just not possible. So I decided to just go all out fixing naming and parameter types.
What I didn't care for at all were internal methods. Many were removed, renamed and/or had the parameter list changed completely. For example the COBS stuff has been condensed into one function, as you essentially searched the data 3 times. My implementation does it all in 1 pass.

In any case, I'll make sure that the examples will be updated one way or another (unless you insist on doing it yourself).

Also, sorry about the squash thing - I'm not a professional software developer. Thank you for the insights, I'm learning a lot!

No hard feelings. It was just a bit of a hassle having to rebase the branch locally.
And I'm very happy to hear that you're learning.

[PowerBroker2]

I looked over everything and it looks good. Since you know the new source code better, you should update the examples. I'll do the acceptance testing on my hardware and merge when ready.

[PowerBroker2]

Thanks, everything compiles fine now

• UART: Working 100%
• SPI: RX sketches are completely unresponsive
• I2C: RX sketches are completely unresponsive

I'll look into what might be going wrong with SPI and I2C

[PowerBroker2]

I2C:

While the TwoWire class inherits from the Stream class, there are a couple of TwoWire members needed for access by this library. These required members include:

// sets function called on slave write
void TwoWire::onReceive( void (*function)(int) )

void TwoWire::beginTransmission(uint8_t address)

and

uint8_t TwoWire::endTransmission(uint8_t sendStop)

SPI:

For SPI, the slave needs to somehow be properly configured with SPCR |= bit (SPE);. Also, SPI uses interrupt ISR (SPI_STC_vect) to handle incoming SPI data.

Also, I noticed your code attempts to write multiple bytes per port->transfer() call. I originally tried this as well, but found that this approach was the only way I could get it to work:

SerialTransfer/src/SPITransfer.cpp

Lines 72 to 90 in 64fc4ea

	digitalWrite(SS, LOW); // Enable SS (active low)
	for (uint8_t i = 0; i < sizeof(packet.preamble); i++)
	{
	delay(1); // This delay is needed
	port->transfer(packet.preamble[i]);
	}
	for (uint8_t i = 0; i < numBytesIncl; i++)
	{
	delay(1); // This delay is needed
	port->transfer(packet.txBuff[i]);
	}
	for (uint8_t i = 0; i < sizeof(packet.postamble); i++)
	{
	delay(1); // This delay is needed
	port->transfer(packet.postamble[i]);
	}
	digitalWrite(SS, HIGH); // Disable SS (active low)

[BrainStone]

Do you really need the onReceive function? I mean the way it has been implemented it doesn't matter at all when the data has been received.

Now also an interesting question for both SPI and I2C is if they are operating in slave or master mode. Because admittedly the way both are implemented it's assumed they are master.

I'll have a little think about how I manage the master slave dilemma. Your input on the matter would be greatly aprechiated. And it's important because both master and slave can send and receive data and frankly both should be able to.
With serial that doesn't matter as both are seen as equal.

[PowerBroker2]

Do you really need the onReceive function? I mean the way it has been implemented it doesn't matter at all when the data has been received.

Yes, I've done some testing on my own and TwoWire.available()/TwoWire.read() calls do not work unless used within the callback specified in the TwoWire.onReceive() function.

Now also an interesting question for both SPI and I2C is if they are operating in slave or master mode. Because admittedly the way both are implemented it's assumed they are master.

Maybe in terms of the library's perspective, but at the sketch level, users are expected to know and configure their Arduino appropriately. I don't think that's necessarily the best way to do it, but it was the easiest way to get it to work originally.

I'll have a little think about how I manage the master slave dilemma. Your input on the matter would be greatly aprechiated. And it's important because both master and slave can send and receive data and frankly both should be able to.
With serial that doesn't matter as both are seen as equal.

Yes, the master/slave concept is an odd problem-set to deal with for this library.

While it may be more difficult, I think it would be best for the begin() method for the I2C and SPI classes to include arguments that allow automatic configurations (master or slave) of the given Arduino. I would also like to expand the I2C/SPI functionality so that master devices can receive and parse packets like you mentioned, but I haven't looked into it enough yet.

Note that there may be some things the users will have to implement at the sketch level to get things to work.

[PowerBroker2]

We also need to add another board (ESP8266) to the following processor exclusion conditions and readme disclaimer:

SerialTransfer/src/SPITransfer.h

Line 5 in 64fc4ea

#if not(defined(MBED_H) || defined(__SAM3X8E__)) // These boards are/will not be supported by SPITransfer.h

SerialTransfer/src/SPITransfer.cpp

Line 3 in 64fc4ea

#if not(defined(MBED_H) || defined(__SAM3X8E__)) // These boards are/will not be supported by SPITransfer.h

[BrainStone]

Taken care of the SPI thing.

And I like the idea of letting the user chose whether or not the library is in slave mode or not.
However due to the complexity of that I'll need a few days to properly handle all that. Especially considering my life's become fairly stressful due to recent unexpected events.
I promise I will finish this but I can't promise to keep working at the same speed.
If you want you can also add commits to this PR if you have good ideas or want to implement an obvious fix.

[baris-ekici]

Seems very good lib, even though I'll test it in UART but uses lots of memory almost half of the memory of 328 and all of the 168. Not much left for other parts of the code.
You should consider trying to reduce this as easytransfer lib use less than 200bytes.

[BrainStone]

It (in this version) should use a bit more than 1kb of ROM and 300 bytes of RAM. What numbers are you seeing?
And did you use the version in this PR or the current release @baris-ekici?

[baris-ekici]

Sketch uses 5678 bytes (18%) of program storage space. Maximum is 30720 bytes.
Global variables use 955 bytes (46%) of dynamic memory

Am I using the wrong one?

[baris-ekici]

this is results of the compiling the uart_rx example by the way.

[BrainStone]

Sketch uses 5678 bytes (18%) of program storage space. Maximum is 30720 bytes.
Global variables use 955 bytes (46%) of dynamic memory

That looks fine to me.

Am I using the wrong one?

I'm not sure which version you downloaded, so I can't tell.

[baris-ekici]

5,6Kb does not a big deal, but it would be good to consider to try to reduce the dynamic memory. 955 bytes lots for just transferring structured data.

[BrainStone]

Again, this has been worked on. If you use the code present in this PR it should be lower.

[brunojoyal]

What is the status of this PR? Many thanks @BrainStone @PowerBroker2

[PowerBroker2]

I've more or less decided to forge on ahead improving the master branch since I couldn't easily grasp what exactly is being updated with this PR. However, that doesn't mean I've given up on it (else I would close it) - maybe I'll revisit sometime in the near future...

@brunojoyal In the mean time, is there any bug you've discovered or have a feature in mind you would like to see in the master branch?

[brunojoyal]

@PowerBroker2,

Thanks a lot for asking, that is nice of you. I am using this library for one of my projects, to transfer photos (roughly 100kb in size) via UART. There is nothing specifically in this pull request that I was looking forward to; mainly I was curious to know the status and direction of development since my project depends on it.

To transfer 100kb files I implemented a small file-transfer protocol using your library. I have a DEVKIT v1 acting as a master and ESPCAM acting as a slave. When at rest, the ESPCAM is listening for SerialTransfer packets which are simply commands encoded in the Packet ID. When it receives a photo request command, it goes to work taking a picture and then sending it over to the master in numbered chunks. It works pretty well considering how primitive it is - perhaps I will make it into its own library. In any case, kudos to you for putting this together, it's been fun to use.

Perhaps it would make sense to have a method to end the transfer? As things are now, I am calling transfer.begin during setup, and my transfer object is then active forever. For the slave, which has to wait to commands, this makes sense. But for the master, it would make more sense to call transfer.begin when a message must be sent or when a message is expected, and to call transfer.end when the message ends. This way, every new transfer starts with a fresh buffer. My greatest difficulty has been to recover gracefully from failed transfers. When a packet fails to be received, sometimes trying to send it almost immediately again works, but sometimes it seems to fail repeatedly until I let the serial buffer time out and/or try enough times. I suspect it is because the remnants of the failed packet sometimes still populate the packet buffer and mess up each new incoming packet. Or something...

Aside from a transfer.end method, another feature which could help keep the buffers clean is some kind of timeout option (separate from the UART buffer timeout): if the whole packet is not successfully received within TIMEOUT μs, then it is marked as a failure and the packet is reset.

What do you think? I hope I'm not trying to reinvent the wheel. Perhaps there are better ways to deal with these things. Please do let me know. In any case, good work!

Oh and one more minor thing, perhaps available() should be called something else, maybe process() or tick()? My feeling comes from the fact that calling available() twice in a row should not destroy anything. As it stands, it crushes any packet received during the first available() call, contrasting with the available() method of the Stream class.

[brunojoyal]

@PowerBroker2.

Please take a look at https://github.com/brunojoyal/SerialTransfer.
I have implemented a packet timeout option in the SerialTransfer and Packet constructors.
Using this fork, with a 50ms timeout on both master and slave, using a baud rate of 230400bps, I have gone from roughly 80% success at my file transfer attempts, to what seems to be 99% success.

I should say that I am using an unshielded unstranded 4-conductor wire to carry both 5V DC and the UART lines, so my serial is prone to error.

Please let me know what you think!

[PowerBroker2]

Thanks a lot for asking, that is nice of you. I am using this library for one of my projects, to transfer photos (roughly 100kb in size) via UART. There is nothing specifically in this pull request that I was looking forward to; mainly I was curious to know the status and direction of development since my project depends on it.

That sounds like a cool project! I'm glad you've found my lib useful. While I'm not doing a whole lot with it right now, it is still very much maintained and active.

Perhaps it would make sense to have a method to end the transfer? As things are now, I am calling transfer.begin during setup, and my transfer object is then active forever. For the slave, which has to wait to commands, this makes sense. But for the master, it would make more sense to call transfer.begin when a message must be sent or when a message is expected, and to call transfer.end when the message ends. This way, every new transfer starts with a fresh buffer. My greatest difficulty has been to recover gracefully from failed transfers.

I'm not sure how useful a transfer.end() would be. If you don't want to parse packets for a certain amount of time, then don't call transfer.available(). A better option would be to make a sort of reset() method that clears out the serial input, tx, and rx buffers, plus resets the "bytes read" variable and finite state machine. This could be automatically called when an error occurs and make it public so that users can call it when they wish.

When a packet fails to be received, sometimes trying to send it almost immediately again works, but sometimes it seems to fail repeatedly until I let the serial buffer time out and/or try enough times. I suspect it is because the remnants of the failed packet sometimes still populate the packet buffer and mess up each new incoming packet. Or something...

Yeah, I'm not sure why this would be an issue, however, I don't think it resets the packet parsing if it comes into contact with a new start byte - I should look into that...

Aside from a transfer.end method, another feature which could help keep the buffers clean is some kind of timeout option (separate from the UART buffer timeout): if the whole packet is not successfully received within TIMEOUT μs, then it is marked as a failure and the packet is reset.

Yeah, I think this would be a good feature!

Oh and one more minor thing, perhaps available() should be called something else, maybe process() or tick()? My feeling comes from the fact that calling available() twice in a row should not destroy anything. As it stands, it crushes any packet received during the first available() call, contrasting with the available() method of the Stream class.

I've never seen anyone call available() in a Stream class more than once before processing the data it's referencing. Either way, I feel it's too late to change such a key piece of the API, plus I like the idea of saying: "a new packet is available()".

Please take a look at https://github.com/brunojoyal/SerialTransfer.

Looks cool! I do have a couple of small edits, but I think you're good to submit a PR as-is. I'll merge and make the small changes I had in mind (nothing big). I am planning on making that reset() method I mentioned earlier.

[brunojoyal]

Thanks a lot for your feedback and comments and for your timely response.

I've submitted a PR with the changes. Cheers!

[brunojoyal]

I'm not sure how useful a transfer.end() would be. If you don't want to parse packets for a certain amount of time, then don't call transfer.available(). A better option would be to make a sort of reset() method that clears out the serial input, tx, and rx buffers, plus resets the "bytes read" variable and finite state machine. This could be automatically called when an error occurs and make it public so that users can call it when they wish.

And I forgot to say, this seems like just what I need!

[PowerBroker2]

I forgot to mention that the lib already has a tick() member function:

SerialTransfer/src/SerialTransfer.cpp

Lines 123 to 143 in 3458920

	/*
	bool SerialTransfer::tick()
	Description:
	------------
	* Checks to see if any packets have been fully parsed. This
	is basically a wrapper around the method "available()" and
	is used primarily in conjunction with callbacks
	Inputs:
	-------
	* void
	Return:
	-------
	* bool - Whether or not a full packet has been parsed
	*/
	bool SerialTransfer::tick()
	{
	if (available())
	return true;
	return false;
	}

[PowerBroker2]

@brunojoyal Take a look at my edits and test them in your project to see if it works. You can clone the current main branch for the latest code. If your tests are successful, I'll tag out a new version.

[brunojoyal]

@PowerBroker2 , I am testing it right now, and it is working like a charm. A considerable improvement since two days ago. Many thanks!

[brunojoyal]

@PowerBroker2 Just an update - my current app has been up for 28 hours, during which it successfully transferred 1345 pictures of roughly 185kb each, with a success rate of 97%. Good stuff!!