Comms protocol post implementation reflection, improving it!

Official FreeEMS vanilla firmware development, the heart and soul of the system!
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Comms protocol post implementation reflection, improving it!

Post by Fred »

Overall, I'd say the serial stuff has been a huge success :-)

However there are some niggles which I would like to address here.

Consider this the public consultation phase :-) Say your piece before I make decisions we all have to live with ;-)

-----------------------------------------------------------------

Number one (my idea) is the proto/firmware bit thing...

Silly idea.

Proposed resolution is to dedicate 256 codes to the protocol (should be heaps) and leave the other 65256 or so for the firmware (will definitely be heaps).

This way will allow you to know its a protocol packet if you want to by reading the upper byte and checking it is zero.
y09223 wrote:Payload Type:
It is unclear to me what the difference between a "protocol" and "firmware" payload type is. Why have two separate sets of payload definitions?
To clarify, the intent was that ALL implementations of the protocol should implement those protocol packet types. This remains the intent, just not with a bit to distinguish, only the range of the ID itself as Stu originally said. The firmware ones on the other hand could be and should be absolutely anything the designer feels like implementing. Unrestrained except by size and speed.

The header bit should go though.

-----------------------------------------------------------------

Number two is reply and ack semantics. Right now things are messy. IE, you might want a reply, or you might not and I might want to give you one (error or otherwise and I might not)

To be blunt, I don't like it.

What I would prefer is to *always* send a reply back (except for resets, but even those could be done via a one off start up message).

This keeps things clean and reduces code.

Currently I think the ack field should remain an option. It will be easy enough to send/not send that.
y09223 wrote:As best I can tell, the need and use of Ack/Seq Number byte is undefined in the spec. Also, in reading through the forum, I could find no obvious argument in favor of it.
The purpose was to ensure that you KNOW that a packet is for you. IE, a packet of the same type sent at much the same time as a second host on a shared connection will elicit the same type of response. The content of the request could have been different though, depending on which type. The only way to KNOW it's for you and not someone else is the matching ack number being sent back. The chances of it being for someone else are then heavily reduced.

Further to that, it could serve as a sequence number to indicate lost packets to the tuning app if they are sent and received sequentially and attempted to be checked for consistency of replies.

------------------------------------------------------------------

Addresses. I think these should stay as it's easy to handle and essential for a networked environment on CAN later IMO.

However :
Addressing Behaviour:
Addressing does not seem to be necessary at the message packet level.<SNIP>If a CAN or LIN bus is to be added to the EMS, CAN and LIN addressing can be easily implemented by defining one of the Payload ID reserved bits to indicated a CAN or LIN bus, with the Payload ID containing the address. The EMS would then simply pass the payload to the bus/address specified, minimizing parsing and processing overhead.
Can you elaborate more on this as I do not have a game plan at the moment. My only thought was "if the packets themselves have the address then it doesn't matter how they get there as long as they do and they will always work the same way." If there is a better way, please either here or the original thread or the take 2 thread (preferably the latter) tell use all about it!

Discussion on CAN stuff and other bits and bobs : http://www.diyefi.org/forum/viewtopic.php?f=8&t=454

-------------------------------------------------------------------

Y09223 pointed out that the bits in the header not used should be reserved, not left open for discussion. Fair enough. Saying they are reserved won't stop me or others using them for experimentation at the end of the day, and will keep things clean into the future.

-------------------------------------------------------------------
y09223 wrote:Payload:
Each packet should contain exactly one payload. Given a payload length of 2^32, there should be no need for multi-packet sequences. By the same logic, a string payload should contain one item; if multiple character blocks are to be sent, each block should be in it's own packet. Besides, null-terminated blocks take time to encode and decode.
Given that we only have 32k of ram, and we only have 512k of flash, and that the chance of filling it all are slim to none, the 65k of packet we can send with a 16 bit (less expensive speed wise) length field seems much more appropriate. Additionally, we can only burn down 1k at a time anyway. The largest single data block currently is 2k and there won't be one over 16k on this platform ever as the flash page window is only 16k and having tables that big is getting absolutely ridiculous. I'm already being accused of making them too big as it is.

I really don't see why we can't put an arbitrary number of strings in one packet. What is the problem there? I definitely don't think the packet should know what it is carrying. Only how long and what it's ID is. If the ID means a certain type, the length isn't even required. The null terminations are not a time killer at all except the bandwidth (minimal) as all strings in C will have one on the end anyway.

I think this is a non issue really. Though I've put it here to allow discussion of it.

--------------------------------------------------------------------------
UART specific items:
STX and ETX characters can appear in a binary data packet. Even with an escaping scheme, the escape sequence can appear in the binary data packet. This is a 30-40 year old problem, with a lot of good solutions out there. But, ALL solutions require additional code (memory) and processing (cycles).
This is not true. Any time they *would* appear they are converted and prefixed with an escape byte. The worst case is a packet made of only special bytes. Worst case is half speed. Normal case is about 0.5% speed hit from this.

----------------------------------------------------------------------------

For the record, I am concerned about bandwidth, but much much less so about CPU cycles and memory, of those there is a reasonable supply. Thank you for your input John!!

----------------------------------------------------------------------------

On more positive notes :

Payload ID works brilliantly!
Header flags work brilliantly!
Start/Stop/Escape work brilliantly!
Checksum works brilliantly!

Due to the above, it's FAST :-) Not slow! To recap, if you don't have this stuff, you have to basically poll... polling = delays/waiting. With this stuff we can back to back messages and still receive them cleanly. This works in practice to give us double or more what MegaSquirt can do ;-)

For those that haven't seen it, here is a vid of the current two matching code bases in action :

http://www.youtube.com/watch?v=4MS_zc8sZxM

I'd love to get your feedback on this, whoever you are!

Regards,

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
sry_not4sale
LQFP144 - On Top Of The Game
Posts: 568
Joined: Mon Mar 31, 2008 12:47 am
Location: New Zealand, land of the long white burnout
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by sry_not4sale »

+1 for proposed payload id changes and removal of header bit
+1 always send reply back!!! please!!!

:D
Owner / Builder: 1983 Mazda Cosmo 12at (1200cc 2-rotor turbo) coupe [SPASTK]
165hp @ 6psi standard - fastest production car in japan Oct 82
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by Fred »

Thanks Aaron! Does anyone else have anything to say? I'll probably just go ahead and make those changes fairly soon if no one pipes up. I think its a sound decision. It would be nice to have those people that have commented in the past through their thoughts at it though.

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by Fred »

Bump for feedback. I'll be attacking this hard out as of monday next week if all goes well (and it should). If anyone has objections now is your chance to voice them.

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by Fred »

I'll be working on this this evening if there are no objections today. Speak now or forever hold your piece!

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by Fred »

I made a start on this tonight. Removed the proto/firm bit and rearranged all the payload ids to suit. I also restored the orange button to reset duties and added some further functions partially. I'll do more on this stuff in the morning and commit + push tomorrow evening.

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
tobz
TO220 - Visibile
Posts: 14
Joined: Sat Apr 25, 2009 10:28 am
Location: Rhode Island, United States
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by tobz »

Maybe a little late, but I like where you're going with this. The only thing I'd change is the header area, and at that rate, how you refer to everything.

It's a little easier for most people who deal with networking code, I'd say, if everything that isn't payload is called the header, and everything that isn't included everytime in a packet is called a payload.

With that said, the packet ID is obviously needed to differentiate payload. That's fine. The idea to use the high byte as a flag, of sorts, probably isn't needed. Depending on how you're routing packet (routing table or long ass switch statement) it's probably just added complexity to look for/people to understand which bytes represent what.

I'd say that whatever other flags you have in the header area are fine. Serial communication is synchronous so there is no need for a sequence to be included. That might be a consideration for CAN, maybe?

The only actual problem I see is how you designate payload. Start/stop is a very rudimentary way to do it. It forces tuner/dattalogging software to constantly look for specific bytes. How is this affecting your payload? It can't be anything but a negative effect. I would suggest you have a size field in the header. This tells the software to preallocate for X bytes and makes it more efficient. Also, I personally had a thought of how the start/stop being split gave more time for bad noise/RFI/EMI/whatever to propogate into your wiring and cause bad signals that essentially never let a proper start/stop byte come thru, leaving your software all gummed up.


Thus, my proposed packet format would be like this:

<payload ID - 2 bytes><any misc. stuff - x bytes><packet size - payload, 2 bytes><payload - x bytes><CRC - 1 byte>

A concessions that can/might have to be made is: the packet size might not be big enough. If not, you could either move to a full 3 byte payload size, or you can implement 2-3 byte expansion. (I'll explain it below)

A way this problem has been handled is something I actually discovered working with the online game World of Warcraft. (I'm sure most of you have heard about it, or played it) The size field is almost always 2 bytes long. This handles packets up to 2^16 length. If you have a bigger packet, it uses 3 bytes to store the length. The first byte is masked 0x80 to flag the client and notify them to read 3 bytes instead of only two. Then, you use that first byte, masked to 0x7F, for the actual size value, plus the remaining two bytes. This lets you send around 8 megabytes of data in a packet.

Psuedo-code:

Code: Select all

int32 packetSize = 0;
int8 firstByte = byteDataArray[i];

if(firstByte & 0x80 != 0)
{
    // big packet.
    packetSize = (firstByte & 0x7F) << 16;
    packetSize |= byteDataArray[i + 1] << 8;
    packetSize |= byteDataArray[i + 2];
}
else
{
    packetSize = firstByte << 8;
    packetSize |= byteDataArray [i + 1];
}
The only problem is this forces you to waste memory as you need a 32-bit value for something that is 24 bits max. At any rate, I can't imagine 2 bytes being a real limitation, and this code would only, I imagine, be a small performance hit anyways.


Thoughts? :?:
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by Fred »

Thoughts :
tobz wrote:Maybe a little late, but I like where you're going with this.
It's never too late! :-) Glad you like the general idea.
It's a little easier for most people who deal with networking code, I'd say, if everything that isn't payload is called the header, and everything that isn't included everytime in a packet is called a payload.
Hmm, perhaps multiple layers of header then? primary core stuff on the outer layer then possibly internal header of some sort and then finally actual payload? I can't even remember the details of what is in there right now. I'll have to take another look soon and post again. I thought we had gone to a fixed header setup anyway :-/ I forget.
The idea to use the high byte as a flag, of sorts, probably isn't needed. Depending on how you're routing packet (routing table or long ass switch statement) it's probably just added complexity to look for/people to understand which bytes represent what.
Well, that is an implementation detail right? They can choose to implement the base standard as it's own abstract thing and extend it for the specific device, or they can munge it into a giant mess that isn't portable across devices. I think it's important to separate what is part of the protocol from what is part of the device spec. I could just say X Y Z payload IDs are protocol ones and the rest are free range, but drawing a nice clean line at 255 seems better and simpler to me. If someone wants to ignore it they can right? Thoughts?
I'd say that whatever other flags you have in the header area are fine. Serial communication is synchronous so there is no need for a sequence to be included. That might be a consideration for CAN, maybe?
This doesn't take away from your outcome of "no need for sequence" (have to think more about that) but I dispute that "serial comms are synchronous" - request/response serial comms are, but what about async packets flying at the PC from the device? what about shared serial lines bouncing packets around in an overlapping fashion? These things are asynchronous and need some sort of logic or control to behave correctly. I'm not sure what that best is, but I'll think more about it soon.
The only actual problem I see is how you designate payload. Start/stop is a very rudimentary way to do it. It forces tuner/dattalogging software to constantly look for specific bytes.
I don't see the problem with it to be honest. You are right that the software must scan the bytes, but the C code I wrote to parse a stream of it showed conclusively that this was a VERY fast operation when done efficiently. I also don't see another way of doing it at all if you want to flow continuous data with no gaps and therefore maximise your bandwidth.
How is this affecting your payload? It can't be anything but a negative effect.
I don't understand how it affects it at all? The worst it can do is make it bloated, but we are talking 3/256 rate of bloat on the average space wise and a worst case of double. The speed hit for creating the escapes is minimal on both sides and the connection can be easily saturated with this system.
I would suggest you have a size field in the header. This tells the software to preallocate for X bytes and makes it more efficient.
Could do, but you then need a separate set of boundaries for the header and a separate checksum and the associated overhead for that. I doubt it is worth it. As for the preallocate thing, that can be taken care of (space inefficiently) by querying the device for it's max packet size and just allocating all of them to that. eg :

device won't send more than 1024 bytes, so either unescape on the fly and buffer up to 1024 OR allocate double for raw data and parse once received. that's stuff all data regardless and garbage collection or explicit memory management will take care of the unused space once the message has been split down to an internal format and packed appropriately there.
Also, I personally had a thought of how the start/stop being split gave more time for bad noise/RFI/EMI/whatever to propogate into your wiring and cause bad signals that essentially never let a proper start/stop byte come thru, leaving your software all gummed up.
I don't understand what you mean by split and time. the point of the stop/start is so you can back to back the messages and there is no time wasted. Besides, the situation with a bad connection is best shown to the user by a slow down. If you don't get a slow down from it you are accepting bad data which could mean a melted piston.
A concessions that can/might have to be made is: the packet size might not be big enough. If not, you could either move to a full 3 byte payload size, or you can implement 2-3 byte expansion. (I'll explain it below)
We were going to do "stretchy ints" but in the end it was decided that there was no point. 2 bytes gets you 64k and that is 1/8 of the flash or more than double the ram, etc. in other words, non issue. Even a bigger better device probably wouldn't want to communicate using larger chunks.
At any rate, I can't imagine 2 bytes being a real limitation
Agreed! :-)

Thanks for your input, I'll have to have a dig through my notes and the code and consider what you have said in more detail.

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
tobz
TO220 - Visibile
Posts: 14
Joined: Sat Apr 25, 2009 10:28 am
Location: Rhode Island, United States
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by tobz »

Firstly, the start/stop thing:

The whole thing about time/split was basically a non-factual based thought that having a designator in different stops gives more of a chance for noise to possibly corrupt the signal, so to speak, and the bytes to change and thus not be recognized, leaving the receiver to not know when the transmission actually stopped.

I'm not an EE by any means so that situation in itself may not even be possible, but it was just a thought.

As far as headers go, in retrospect, drawing that line of 0x00 to 0xFF being for the device is fine, I'd say. I don't know why I thought I had a problem with that. Damn heat...

However, I'd still say the start/stop markers are not the *best* choice. One thing I forgot to mention was that you have a chance for payload to match the markers, so you have to be careful when constructing packets. If you simply put the size in the header, then there is no question of how long a packet is and it's super simple to parse. It's just as easy to construct a header this way. This is also close to a lot of network protocols, so I'd say it'll be a lot easier for people to understand/work with that are coming from other projects doing any sort of network communication. Having worked on two client/server projects, and with a wideband controller, and not to mention the other numerous projects I've looked at.. size in the header or fixed structures are the recurring theme.

What I was talking about with the not needing a sequence is that the communication isn't asynchronous in the sense that you have multiple devices talking to you on that line. If you were relying on sequence numbers to track devices, I'd say OK. Realistically though, what benefit does it give you? The only thing I could figure is that you watch it for any gaps in the sequence ID from packets from the ECU. Maybe disconnect if you saw a gap because we'd assume it was an error. (I honestly couldn't see this being the reason though since unimportant packets, like datalogging, could skip and then you'd totally disconnect for that? it'd be silly)
User avatar
Fred
Moderator
Posts: 15431
Joined: Tue Jan 15, 2008 2:31 pm
Location: Home sweet home!
Contact:

Re: Comms protocol post implementation reflection, improving it!

Post by Fred »

tobz wrote:The whole thing about time/split was basically a non-factual based thought that having a designator in different stops gives more of a chance for noise to possibly corrupt the signal, so to speak, and the bytes to change and thus not be recognized, leaving the receiver to not know when the transmission actually stopped.

I'm not an EE by any means so that situation in itself may not even be possible, but it was just a thought.
You are also assuming that the stream stops, often it won't. continuous bytes with varying meaning. you need something to mark the boundary of these, "no bytes" is not acceptable, you can't max out the connection like that.
As far as headers go, in retrospect, drawing that line of 0x00 to 0xFF being for the device is fine, I'd say. I don't know why I thought I had a problem with that. Damn heat...
It could be unnecessary, but it shouldn't hurt :-)
However, I'd still say the start/stop markers are not the *best* choice.
What is the best choice, given a constant stream of data. Tell me your scheme to determine where packets are in it and I'll consider it.
One thing I forgot to mention was that you have a chance for payload to match the markers, so you have to be careful when constructing packets. If you simply put the size in the header, then there is no question of how long a packet is and it's super simple to parse.
The thing is, you have the payload ID in the header and for the cases where that payload ID *means* a fixed length, why include the length too? For those cases where it doesn't, length is compulsory and will be included. The code to parse this stuff is not tricky as is. It just needs to be done in appropriate stages :-)
It's just as easy to construct a header this way. This is also close to a lot of network protocols, so I'd say it'll be a lot easier for people to understand/work with that are coming from other projects doing any sort of network communication. Having worked on two client/server projects, and with a wideband controller, and not to mention the other numerous projects I've looked at.. size in the header or fixed structures are the recurring theme.
Fair enough, I'll definitely consider it. I'm just loath to waste bandwidth when it is so fundamentally limited on the serial line and when the information is already there.
What I was talking about with the not needing a sequence is that the communication isn't asynchronous in the sense that you have multiple devices talking to you on that line. If you were relying on sequence numbers to track devices, I'd say OK. Realistically though, what benefit does it give you? The only thing I could figure is that you watch it for any gaps in the sequence ID from packets from the ECU. Maybe disconnect if you saw a gap because we'd assume it was an error. (I honestly couldn't see this being the reason though since unimportant packets, like datalogging, could skip and then you'd totally disconnect for that? it'd be silly)
Yep, fair call. We were considering that, but not sure what was decided. I need to do some reading, perhaps I'll disable the CSS on the site tomorrow and have a read at work. I've been told to do menial tasks all week anyway, and the thing they want me to do is less than half a days worth, so i have some spare time...

Bed!

Fred.
DIYEFI.org - where Open Source means Open Source, and Free means Freedom
FreeEMS.org - the open source engine management system
FreeEMS dev diary and its comments thread and my turbo truck!
n00bs, do NOT PM or email tech questions! Use the forum!
The ever growing list of FreeEMS success stories!
Post Reply