Fred's unofficial TODO list from his favorite nitpicker

[dandruczyk]

Make the generic "Get" functions (retrieveBlockFromRAM/retrieveBlockFromFlash) take on the offset/length parameters so we can read any part of any location ID.

The Start/Stop Datalog stream functions use a NONSTANDARD API.
These calls use an additional byte after the payload ID which is not defined in the header and breaks a lot of things unnecessarily. Since datalog streaming can be one either ON or OFF use two separate commands instead the current hybrid method. I knew I noticed something funky when looking at the hex editor output of the test commands for configure.basic.datalogs.off and configure.basic.datalog.on
they are 7 bytes, versus most of the other commands are 6 bytes (assuming 00 for header byte):

AA 00 01 94 00 95 CC Datalogs on
and
AA 00 01 94 01 96 CC Datalogs off

Header has NO bits set, so there should be NOTHING after the payload ID (404) except for the checksum and STOP, however you have an additional undocumented byte in there, which appears to act as a boolean.

I suggest changing this to:
AA 00 01 94 95 CC Disable datalog streaming (payload id 404)
AA 00 01 96 97 CC Enable datalog streaming (payload id 406)


Provide a new function to query the LIST of location ID's and/or their lengths.
PayloadID: RequestLocationIDInfo (no params)
Returns: List of location IDs and their lengths, where location ID and length are 16 bit values.

This allows the tuning software to NOT necessarily have to rely on a set in stone beforehand memory layout from fred, (of which i have zero info about becasue I suspect its not well defined yet). Since the JSON/XML/whatever defining the data layouts are not written/designed yet as far as I know, having this will at least give the tuning software the ability to create an in-RAM representation of the ECU's memory and be able to download all data from the ECU to be backed up or otherwise manipulated, maybe via a generic hex editor functionality (which lies dormant in the bowels of megatunix), without forcing fred to define everything right up front.


Nits: Change "size" in the documentation to "length". fred doesn't agree here, and its only semantics, but its definition in freems docs conflicts with docs in megatunix which uses "length" and "size" in megatuni docs represents a variable's size (8/16/32 bit signed/unsigned).

[Fred]

Make the generic "Get" functions (retrieveBlockFromRAM/retrieveBlockFromFlash) take on the offset/length parameters so we can read any part of any location ID.

Probably fine.

The Start/Stop Datalog stream functions use a NONSTANDARD API.
These calls use an additional byte after the payload ID which is not defined in the header and breaks a lot of things unnecessarily. Since datalog streaming can be one either ON or OFF use two separate commands instead the current hybrid method. I knew I noticed something funky when looking at the hex editor output of the test commands for configure.basic.datalogs.off and configure.basic.datalog.on
they are 7 bytes, versus most of the other commands are 6 bytes (assuming 00 for header byte):

AA 00 01 94 00 95 CC Datalogs on
and
AA 00 01 94 01 96 CC Datalogs off

Header has NO bits set, so there should be NOTHING after the payload ID (404) except for the checksum and STOP, however you have an additional undocumented byte in there, which appears to act as a boolean.

I suggest changing this to:
AA 00 01 94 95 CC Disable datalog streaming (payload id 404)
AA 00 01 96 97 CC Enable datalog streaming (payload id 406)

I will fix the documentation and packet names to avoid confusion like this. The code is good, though.

Provide a new function to query the LIST of location ID's and/or their lengths.
PayloadID: RequestLocationIDInfo (no params)
Returns: List of location IDs and their lengths, where location ID and length are 16 bit values.

This allows the tuning software to NOT necessarily have to rely on a set in stone beforehand memory layout from fred, (of which i have zero info about becasue I suspect its not well defined yet). Since the JSON/XML/whatever defining the data layouts are not written/designed yet as far as I know, having this will at least give the tuning software the ability to create an in-RAM representation of the ECU's memory and be able to download all data from the ECU to be backed up or otherwise manipulated, maybe via a generic hex editor functionality (which lies dormant in the bowels of megatunix), without forcing fred to define everything right up front.

This isn't necessarily a bad idea, we'll see. It has some issues, but everything I can think of surrounding this aspect of defining the firmware data layout currently has some issues. This could turn out to be the best approach. We'll talk about it and I'll think about it.

Nits: Change "size" in the documentation to "length". fred doesn't agree here, and its only semantics, but its definition in freems docs conflicts with docs in megatunix which uses "length" and "size" in megatuni docs represents a variable's size (8/16/32 bit signed/unsigned).

Nits indeed :-p :-)

Fred.

[Fred]

The documentation was good.

Title: setAsyncDatalogType
Description: Configure the asynchronous datalog type 0x00/0 is off, other values turn specific types of logging on.
Payload ID: 0x0194/404
Length: 1
Contents: datalogType (1)

I changed the example packet names to be more useful and less misleading (not that they were bad before... but...)

Fred.

[Fred]

Just to be clear, this IS a generic interface. What you're asking for (whether you realise it or not) is a payload type per datalog type, and there could be 255 of those! It would be easy to do, as it would just be a mapping between payload ID and an 8 bit argument to a function, just as if you sent the 8 biut argument yourself, but it would be horrible and hacky and ugly, on both sides.

[Fred]

Header has NO bits set, so there should be NOTHING after the payload ID (404) except for the checksum and STOP, however you have an additional undocumented byte in there, which appears to act as a boolean.

Just to be clear, all the other packet types with arguments also have 0x00 for the header! The header does not define the request payload contents, the payload ID does! that payload ID DOES define that there will be a byte there, and exactly what it means.

Having said that, I put the datalog control packet into the system so I could control the datalogs. At the time the partial updates mechanism did not exist and I had no design for it in mind as I wanted to keep the option of verification alive and hadn't thought of a good way. Currently it throws an error if the datalog type is no good, but that isn't necessary, silence is a legitimate option. In that case, there is no reason why the datalogs can't be configured in a normal ram/flash field just like any other config parameter, maybe they already are? I'd have to check. I might make that change today and remove this packet so that it's a single byte in the config which represents the type to stream, 0x00 and all undefined bytes will produce no stream. I might make undefined bytes send an error and reconfigure themselves to zero again, we'll see.

Do you want the datalog configure command, as is? or is the ram/flash field enough/better? I think it is, so I'd like to redo my test packets and remove this part of the interface if possible. Thoughts?

Fred.

[Fred]

The datalog stream type can be configured through both the dedicated payload ID above that was clean enough, but pointless now, and the standard partial packet update mechanism. Test packets are available. I will write one more before bed and that is to burn the change to flash so after reset the new behaviour sticks.

If you don't want the dedicated one, and I suspect you wont, then I will remove it soon, as there is no longer a need for it. The field being changed in ram is the same as is available through the more general interface.

Fred.

[dandruczyk]

Via ram/flash GENERIC setter is better I think as long as it actually sets something in ram/flash. I'm up in the air, as in that case I'll hjave to worry about a burn to flash call and other overhead. I have it working with the API I was bitching about so that can stay, as i misintepreted your split docs (docs in docs and docs in interface). IMHO all docs should be under ONE hiearchy, if you need to separated them use a subdir under docs, not in some other place.

[Fred]

Done. I can't see a point in keeping the old interface, but it doesn't hurt to leave it there until you migrate, i guess, and if you never do, also no big deal. It seems nice to be able to burn that datalog type down to flash for the future, though.

[Fred]

Make the generic "Get" functions (retrieveBlockFromRAM/retrieveBlockFromFlash) take on the offset/length parameters so we can read any part of any location ID.

This is done (and tested!), but I forgot to post here about it.

Provide a new function to query the LIST of location ID's and/or their lengths.
PayloadID: RequestLocationIDInfo (no params)
Returns: List of location IDs and their lengths, where location ID and length are 16 bit values.

I just thought of a not-totally-ugly way of doing this, so I will implement it for you soon. I don't know how fast this packet will be to respond, though, it could take a little time, probably not much in human terms, though. Also don't forget that there are parent sections and child sections to any number of levels (however I could limit myself to two and see no reason not to). IE, you'll get duplicate data from this using this function and pulling/pushing everything. We could do more than one variation too, IE, the things required to tune OR the things required to do a full image most quickly and with least flash memory wear. Perhaps I could add a "parentID" field to the internal database and only send them if it is zero (for the purposes of imaging the ecu contents). I just realised a potential problem. The max packet size is 2k, so it limits us to a max of 500 payloadid/size-length pairs before we'd have to send back multiple packets and currently the spec and implementation dont allow for that sort of behaviour. I doubt we'll really hit it (currently only 30 or less) but it's something to keep in mind extensibility wise.

having this will at least give the tuning software the ability to create an in-RAM representation of the ECU's memory and be able to download all data from the ECU to be backed up or otherwise manipulated, maybe via a generic hex editor functionality (which lies dormant in the bowels of megatunix), without forcing fred to define everything right up front.

A hex editor and full data dump or image dump or tunable dump would be gold for prototyping new features without bothering you or hacking at mtx myself! :-) Win Win. I'll wait to see what you say about the mix of full page ids and partial tunable ids and separating them or providing parent info along with length before I go to far with it. The implementation shouldn't take too long, but I won't start until I know what you want.

Fred.

[dandruczyk]

Hmm, well think about it and gt back to me. In my head I like the idea of the tuning software being able to ask the ecu" what do you have for a basic memory layout, i.e. how many pages and what size per page. This way I can query lal that and store it and give yo ua raw editor without having to worry about any special info for tab design. It'll be cryptic and rough for editing but theoretically give you access to any point in memory.

I'm of the opinion it should provide ONLY the location ID's that can be read/written to via the GENERIC functions with no restrictions, unlike your special purpose get/set'rs. you've said before these should cover ALL places in memory, and that your object specific getters and setters refer to the same places but have checks wrapped around them.