This imports Tiny AES128 from https://github.com/kokke/tiny-AES128-C for use in the decoding of OSTC3 firmwares.
This aes-code is released into the public domain.
Signed-off-by: Anton Lundin glance@acc.umu.se --- msvc/libdivecomputer.vcproj | 6 + src/Makefile.am | 1 + src/aes.c | 486 ++++++++++++++++++++++++++++++++++++++++++++ src/aes.h | 16 ++ 4 files changed, 509 insertions(+) create mode 100644 src/aes.c create mode 100644 src/aes.h
diff --git a/msvc/libdivecomputer.vcproj b/msvc/libdivecomputer.vcproj index 78a8190..10a4cf7 100644 --- a/msvc/libdivecomputer.vcproj +++ b/msvc/libdivecomputer.vcproj @@ -247,6 +247,9 @@ > </File> <File + RelativePath="..\src\aes.c" + > + <File RelativePath="..\src\hw_ostc3.c" > </File> @@ -537,6 +540,9 @@ > </File> <File + RelativePath="..\src\aes.h" + > + <File RelativePath="..\include\libdivecomputer\hw_ostc3.h" > </File> diff --git a/src/Makefile.am b/src/Makefile.am index 595f0c7..eb39a4b 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -43,6 +43,7 @@ libdivecomputer_la_SOURCES = \ ihex.h ihex.c \ hw_ostc.c hw_ostc_parser.c \ hw_frog.c \ + aes.h aes.c \ hw_ostc3.c \ cressi_edy.c cressi_edy_parser.c \ cressi_leonardo.c cressi_leonardo_parser.c \ diff --git a/src/aes.c b/src/aes.c new file mode 100644 index 0000000..000a067 --- /dev/null +++ b/src/aes.c @@ -0,0 +1,486 @@ +/* + +This is an implementation of the AES128 algorithm, specifically ECB mode. + +The implementation is verified against the test vectors in: + National Institute of Standards and Technology Special Publication 800-38A 2001 ED + +ECB-AES128 +---------- + + plain-text: + 6bc1bee22e409f96e93d7e117393172a + ae2d8a571e03ac9c9eb76fac45af8e51 + 30c81c46a35ce411e5fbc1191a0a52ef + f69f2445df4f9b17ad2b417be66c3710 + + key: + 2b7e151628aed2a6abf7158809cf4f3c + + resulting cipher + 50fe67cc996d32b6da0937e99bafec60 + d9a4dada0892239f6b8b3d7680e15674 + a78819583f0308e7a6bf36b1386abf23 + c6d3416d29165c6fcb8e51a227ba994e + + +NOTE: String length must be evenly divisible by 16byte (str_len % 16 == 0) + You should pad the end of the string with zeros if this is not the case. + +*/ + +#ifndef _AES_C_ +#define _AES_C_ + + +/*****************************************************************************/ +/* Includes: */ +/*****************************************************************************/ +#include <stdint.h> +#include "aes.h" + + +/*****************************************************************************/ +/* Defines: */ +/*****************************************************************************/ +// The number of columns comprising a state in AES. This is a constant in AES. Value=4 +#define Nb 4 +// The number of 32 bit words in a key. +#define Nk 4 +// Key length in bytes [128 bit] +#define keyln 16 +// The number of rounds in AES Cipher. +#define Nr 10 + + +/*****************************************************************************/ +/* Private variables: */ +/*****************************************************************************/ +// in - pointer to the CipherText to be decrypted. +// out - pointer to buffer to hold output of the decryption. +// state - array holding the intermediate results during decryption. +static uint8_t* in, *out, state[4][4]; + +// The array that stores the round keys. +static uint8_t RoundKey[176]; + +// The Key input to the AES Program +static uint8_t* Key; + +// The lookup-tables are marked const so they can be placed in read-only storage instead of RAM +// The numbers below can be computed dynamically trading ROM for RAM - +// This can be useful in (embedded) bootloader applications, where ROM is often limited. +static const uint8_t sbox[256] = { + //0 1 2 3 4 5 6 7 8 9 A B C D E F + 0x63, 0x7c, 0x77, 0x7b, 0xf2, 0x6b, 0x6f, 0xc5, 0x30, 0x01, 0x67, 0x2b, 0xfe, 0xd7, 0xab, 0x76, + 0xca, 0x82, 0xc9, 0x7d, 0xfa, 0x59, 0x47, 0xf0, 0xad, 0xd4, 0xa2, 0xaf, 0x9c, 0xa4, 0x72, 0xc0, + 0xb7, 0xfd, 0x93, 0x26, 0x36, 0x3f, 0xf7, 0xcc, 0x34, 0xa5, 0xe5, 0xf1, 0x71, 0xd8, 0x31, 0x15, + 0x04, 0xc7, 0x23, 0xc3, 0x18, 0x96, 0x05, 0x9a, 0x07, 0x12, 0x80, 0xe2, 0xeb, 0x27, 0xb2, 0x75, + 0x09, 0x83, 0x2c, 0x1a, 0x1b, 0x6e, 0x5a, 0xa0, 0x52, 0x3b, 0xd6, 0xb3, 0x29, 0xe3, 0x2f, 0x84, + 0x53, 0xd1, 0x00, 0xed, 0x20, 0xfc, 0xb1, 0x5b, 0x6a, 0xcb, 0xbe, 0x39, 0x4a, 0x4c, 0x58, 0xcf, + 0xd0, 0xef, 0xaa, 0xfb, 0x43, 0x4d, 0x33, 0x85, 0x45, 0xf9, 0x02, 0x7f, 0x50, 0x3c, 0x9f, 0xa8, + 0x51, 0xa3, 0x40, 0x8f, 0x92, 0x9d, 0x38, 0xf5, 0xbc, 0xb6, 0xda, 0x21, 0x10, 0xff, 0xf3, 0xd2, + 0xcd, 0x0c, 0x13, 0xec, 0x5f, 0x97, 0x44, 0x17, 0xc4, 0xa7, 0x7e, 0x3d, 0x64, 0x5d, 0x19, 0x73, + 0x60, 0x81, 0x4f, 0xdc, 0x22, 0x2a, 0x90, 0x88, 0x46, 0xee, 0xb8, 0x14, 0xde, 0x5e, 0x0b, 0xdb, + 0xe0, 0x32, 0x3a, 0x0a, 0x49, 0x06, 0x24, 0x5c, 0xc2, 0xd3, 0xac, 0x62, 0x91, 0x95, 0xe4, 0x79, + 0xe7, 0xc8, 0x37, 0x6d, 0x8d, 0xd5, 0x4e, 0xa9, 0x6c, 0x56, 0xf4, 0xea, 0x65, 0x7a, 0xae, 0x08, + 0xba, 0x78, 0x25, 0x2e, 0x1c, 0xa6, 0xb4, 0xc6, 0xe8, 0xdd, 0x74, 0x1f, 0x4b, 0xbd, 0x8b, 0x8a, + 0x70, 0x3e, 0xb5, 0x66, 0x48, 0x03, 0xf6, 0x0e, 0x61, 0x35, 0x57, 0xb9, 0x86, 0xc1, 0x1d, 0x9e, + 0xe1, 0xf8, 0x98, 0x11, 0x69, 0xd9, 0x8e, 0x94, 0x9b, 0x1e, 0x87, 0xe9, 0xce, 0x55, 0x28, 0xdf, + 0x8c, 0xa1, 0x89, 0x0d, 0xbf, 0xe6, 0x42, 0x68, 0x41, 0x99, 0x2d, 0x0f, 0xb0, 0x54, 0xbb, 0x16 }; + +static const uint8_t rsbox[256] = +{ 0x52, 0x09, 0x6a, 0xd5, 0x30, 0x36, 0xa5, 0x38, 0xbf, 0x40, 0xa3, 0x9e, 0x81, 0xf3, 0xd7, 0xfb +, 0x7c, 0xe3, 0x39, 0x82, 0x9b, 0x2f, 0xff, 0x87, 0x34, 0x8e, 0x43, 0x44, 0xc4, 0xde, 0xe9, 0xcb +, 0x54, 0x7b, 0x94, 0x32, 0xa6, 0xc2, 0x23, 0x3d, 0xee, 0x4c, 0x95, 0x0b, 0x42, 0xfa, 0xc3, 0x4e +, 0x08, 0x2e, 0xa1, 0x66, 0x28, 0xd9, 0x24, 0xb2, 0x76, 0x5b, 0xa2, 0x49, 0x6d, 0x8b, 0xd1, 0x25 +, 0x72, 0xf8, 0xf6, 0x64, 0x86, 0x68, 0x98, 0x16, 0xd4, 0xa4, 0x5c, 0xcc, 0x5d, 0x65, 0xb6, 0x92 +, 0x6c, 0x70, 0x48, 0x50, 0xfd, 0xed, 0xb9, 0xda, 0x5e, 0x15, 0x46, 0x57, 0xa7, 0x8d, 0x9d, 0x84 +, 0x90, 0xd8, 0xab, 0x00, 0x8c, 0xbc, 0xd3, 0x0a, 0xf7, 0xe4, 0x58, 0x05, 0xb8, 0xb3, 0x45, 0x06 +, 0xd0, 0x2c, 0x1e, 0x8f, 0xca, 0x3f, 0x0f, 0x02, 0xc1, 0xaf, 0xbd, 0x03, 0x01, 0x13, 0x8a, 0x6b +, 0x3a, 0x91, 0x11, 0x41, 0x4f, 0x67, 0xdc, 0xea, 0x97, 0xf2, 0xcf, 0xce, 0xf0, 0xb4, 0xe6, 0x73 +, 0x96, 0xac, 0x74, 0x22, 0xe7, 0xad, 0x35, 0x85, 0xe2, 0xf9, 0x37, 0xe8, 0x1c, 0x75, 0xdf, 0x6e +, 0x47, 0xf1, 0x1a, 0x71, 0x1d, 0x29, 0xc5, 0x89, 0x6f, 0xb7, 0x62, 0x0e, 0xaa, 0x18, 0xbe, 0x1b +, 0xfc, 0x56, 0x3e, 0x4b, 0xc6, 0xd2, 0x79, 0x20, 0x9a, 0xdb, 0xc0, 0xfe, 0x78, 0xcd, 0x5a, 0xf4 +, 0x1f, 0xdd, 0xa8, 0x33, 0x88, 0x07, 0xc7, 0x31, 0xb1, 0x12, 0x10, 0x59, 0x27, 0x80, 0xec, 0x5f +, 0x60, 0x51, 0x7f, 0xa9, 0x19, 0xb5, 0x4a, 0x0d, 0x2d, 0xe5, 0x7a, 0x9f, 0x93, 0xc9, 0x9c, 0xef +, 0xa0, 0xe0, 0x3b, 0x4d, 0xae, 0x2a, 0xf5, 0xb0, 0xc8, 0xeb, 0xbb, 0x3c, 0x83, 0x53, 0x99, 0x61 +, 0x17, 0x2b, 0x04, 0x7e, 0xba, 0x77, 0xd6, 0x26, 0xe1, 0x69, 0x14, 0x63, 0x55, 0x21, 0x0c, 0x7d }; + + +// The round constant word array, Rcon[i], contains the values given by +// x to th e power (i-1) being powers of x (x is denoted as {02}) in the field GF(2^8) +// Note that i starts at 1, not 0). +static const uint8_t Rcon[255] = { + 0x8d, 0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80, 0x1b, 0x36, 0x6c, 0xd8, 0xab, 0x4d, 0x9a, + 0x2f, 0x5e, 0xbc, 0x63, 0xc6, 0x97, 0x35, 0x6a, 0xd4, 0xb3, 0x7d, 0xfa, 0xef, 0xc5, 0x91, 0x39, + 0x72, 0xe4, 0xd3, 0xbd, 0x61, 0xc2, 0x9f, 0x25, 0x4a, 0x94, 0x33, 0x66, 0xcc, 0x83, 0x1d, 0x3a, + 0x74, 0xe8, 0xcb, 0x8d, 0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80, 0x1b, 0x36, 0x6c, 0xd8, + 0xab, 0x4d, 0x9a, 0x2f, 0x5e, 0xbc, 0x63, 0xc6, 0x97, 0x35, 0x6a, 0xd4, 0xb3, 0x7d, 0xfa, 0xef, + 0xc5, 0x91, 0x39, 0x72, 0xe4, 0xd3, 0xbd, 0x61, 0xc2, 0x9f, 0x25, 0x4a, 0x94, 0x33, 0x66, 0xcc, + 0x83, 0x1d, 0x3a, 0x74, 0xe8, 0xcb, 0x8d, 0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80, 0x1b, + 0x36, 0x6c, 0xd8, 0xab, 0x4d, 0x9a, 0x2f, 0x5e, 0xbc, 0x63, 0xc6, 0x97, 0x35, 0x6a, 0xd4, 0xb3, + 0x7d, 0xfa, 0xef, 0xc5, 0x91, 0x39, 0x72, 0xe4, 0xd3, 0xbd, 0x61, 0xc2, 0x9f, 0x25, 0x4a, 0x94, + 0x33, 0x66, 0xcc, 0x83, 0x1d, 0x3a, 0x74, 0xe8, 0xcb, 0x8d, 0x01, 0x02, 0x04, 0x08, 0x10, 0x20, + 0x40, 0x80, 0x1b, 0x36, 0x6c, 0xd8, 0xab, 0x4d, 0x9a, 0x2f, 0x5e, 0xbc, 0x63, 0xc6, 0x97, 0x35, + 0x6a, 0xd4, 0xb3, 0x7d, 0xfa, 0xef, 0xc5, 0x91, 0x39, 0x72, 0xe4, 0xd3, 0xbd, 0x61, 0xc2, 0x9f, + 0x25, 0x4a, 0x94, 0x33, 0x66, 0xcc, 0x83, 0x1d, 0x3a, 0x74, 0xe8, 0xcb, 0x8d, 0x01, 0x02, 0x04, + 0x08, 0x10, 0x20, 0x40, 0x80, 0x1b, 0x36, 0x6c, 0xd8, 0xab, 0x4d, 0x9a, 0x2f, 0x5e, 0xbc, 0x63, + 0xc6, 0x97, 0x35, 0x6a, 0xd4, 0xb3, 0x7d, 0xfa, 0xef, 0xc5, 0x91, 0x39, 0x72, 0xe4, 0xd3, 0xbd, + 0x61, 0xc2, 0x9f, 0x25, 0x4a, 0x94, 0x33, 0x66, 0xcc, 0x83, 0x1d, 0x3a, 0x74, 0xe8, 0xcb }; + + +/*****************************************************************************/ +/* Private functions: */ +/*****************************************************************************/ +static uint8_t getSBoxValue(uint8_t num) +{ + return sbox[num]; +} + +static uint8_t getSBoxInvert(uint8_t num) +{ + return rsbox[num]; +} + + +// This function produces Nb(Nr+1) round keys. The round keys are used in each round to decrypt the states. +static void KeyExpansion() +{ + uint32_t i, j, k; + uint8_t tempa[4]; // used for the column/row operations + + // The first round key is the key itself. + for(i = 0; i < Nk; ++i) + { + RoundKey[(i * 4) + 0] = Key[(i * 4) + 0]; + RoundKey[(i * 4) + 1] = Key[(i * 4) + 1]; + RoundKey[(i * 4) + 2] = Key[(i * 4) + 2]; + RoundKey[(i * 4) + 3] = Key[(i * 4) + 3]; + } + + // All other round keys are found from the previous round keys. + for(; (i < (Nb * (Nr + 1))); ++i) + { + for(j = 0; j < 4; ++j) + { + tempa[j]=RoundKey[(i-1) * 4 + j]; + } + if (i % Nk == 0) + { + // This function rotates the 4 bytes in a word to the left once. + // [a0,a1,a2,a3] becomes [a1,a2,a3,a0] + + // Function RotWord() + { + k = tempa[0]; + tempa[0] = tempa[1]; + tempa[1] = tempa[2]; + tempa[2] = tempa[3]; + tempa[3] = k; + } + + // SubWord() is a function that takes a four-byte input word and + // applies the S-box to each of the four bytes to produce an output word. + + // Function Subword() + { + tempa[0] = getSBoxValue(tempa[0]); + tempa[1] = getSBoxValue(tempa[1]); + tempa[2] = getSBoxValue(tempa[2]); + tempa[3] = getSBoxValue(tempa[3]); + } + + tempa[0] = tempa[0] ^ Rcon[i/Nk]; + } + else if (Nk > 6 && i % Nk == 4) + { + // Function Subword() + { + tempa[0] = getSBoxValue(tempa[0]); + tempa[1] = getSBoxValue(tempa[1]); + tempa[2] = getSBoxValue(tempa[2]); + tempa[3] = getSBoxValue(tempa[3]); + } + } + RoundKey[i * 4 + 0] = RoundKey[(i - Nk) * 4 + 0] ^ tempa[0]; + RoundKey[i * 4 + 1] = RoundKey[(i - Nk) * 4 + 1] ^ tempa[1]; + RoundKey[i * 4 + 2] = RoundKey[(i - Nk) * 4 + 2] ^ tempa[2]; + RoundKey[i * 4 + 3] = RoundKey[(i - Nk) * 4 + 3] ^ tempa[3]; + } +} + +// This function adds the round key to state. +// The round key is added to the state by an XOR function. +static void AddRoundKey(uint8_t round) +{ + uint8_t i,j; + for(i=0;i<4;i++) + { + for(j = 0; j < 4; ++j) + { + state[j][i] ^= RoundKey[round * Nb * 4 + i * Nb + j]; + } + } +} + +// The SubBytes Function Substitutes the values in the +// state matrix with values in an S-box. +static void SubBytes() +{ + uint8_t i, j; + for(i = 0; i < 4; ++i) + { + for(j = 0; j < 4; ++j) + { + state[i][j] = getSBoxValue(state[i][j]); + } + } +} + +// The ShiftRows() function shifts the rows in the state to the left. +// Each row is shifted with different offset. +// Offset = Row number. So the first row is not shifted. +static void ShiftRows() +{ + uint8_t temp; + + // Rotate first row 1 columns to left + temp = state[1][0]; + state[1][0] = state[1][1]; + state[1][1] = state[1][2]; + state[1][2] = state[1][3]; + state[1][3] = temp; + + // Rotate second row 2 columns to left + temp = state[2][0]; + state[2][0] = state[2][2]; + state[2][2] = temp; + + temp = state[2][1]; + state[2][1] = state[2][3]; + state[2][3] = temp; + + // Rotate third row 3 columns to left + temp = state[3][0]; + state[3][0] = state[3][3]; + state[3][3] = state[3][2]; + state[3][2] = state[3][1]; + state[3][1] = temp; +} + +static uint8_t xtime(uint8_t x) +{ + return ((x<<1) ^ (((x>>7) & 1) * 0x1b)); +} + +// MixColumns function mixes the columns of the state matrix +static void MixColumns() +{ + uint8_t i; + uint8_t Tmp,Tm,t; + for(i = 0; i < 4; ++i) + { + t = state[0][i]; + Tmp = state[0][i] ^ state[1][i] ^ state[2][i] ^ state[3][i] ; + Tm = state[0][i] ^ state[1][i] ; Tm = xtime(Tm); state[0][i] ^= Tm ^ Tmp ; + Tm = state[1][i] ^ state[2][i] ; Tm = xtime(Tm); state[1][i] ^= Tm ^ Tmp ; + Tm = state[2][i] ^ state[3][i] ; Tm = xtime(Tm); state[2][i] ^= Tm ^ Tmp ; + Tm = state[3][i] ^ t ; Tm = xtime(Tm); state[3][i] ^= Tm ^ Tmp ; + } +} + +// Multiplty is a macro used to multiply numbers in the field GF(2^8) +#define Multiply(x,y) (((y & 1) * x) ^ ((y>>1 & 1) * xtime(x)) ^ ((y>>2 & 1) * xtime(xtime(x))) ^ ((y>>3 & 1) * xtime(xtime(xtime(x)))) ^ ((y>>4 & 1) * xtime(xtime(xtime(xtime(x)))))) + + +// MixColumns function mixes the columns of the state matrix. +// The method used to multiply may be difficult to understand for the inexperienced. +// Please use the references to gain more information. +static void InvMixColumns() +{ + int i; + uint8_t a,b,c,d; + for(i=0;i<4;i++) + { + + a = state[0][i]; + b = state[1][i]; + c = state[2][i]; + d = state[3][i]; + + + state[0][i] = Multiply(a, 0x0e) ^ Multiply(b, 0x0b) ^ Multiply(c, 0x0d) ^ Multiply(d, 0x09); + state[1][i] = Multiply(a, 0x09) ^ Multiply(b, 0x0e) ^ Multiply(c, 0x0b) ^ Multiply(d, 0x0d); + state[2][i] = Multiply(a, 0x0d) ^ Multiply(b, 0x09) ^ Multiply(c, 0x0e) ^ Multiply(d, 0x0b); + state[3][i] = Multiply(a, 0x0b) ^ Multiply(b, 0x0d) ^ Multiply(c, 0x09) ^ Multiply(d, 0x0e); + } +} + + +// The SubBytes Function Substitutes the values in the +// state matrix with values in an S-box. +static void InvSubBytes() +{ + uint8_t i,j; + for(i=0;i<4;i++) + { + for(j=0;j<4;j++) + { + state[i][j] = getSBoxInvert(state[i][j]); + } + } +} + + +static void InvShiftRows() +{ + uint8_t temp; + + // Rotate first row 1 columns to right + temp=state[1][3]; + state[1][3]=state[1][2]; + state[1][2]=state[1][1]; + state[1][1]=state[1][0]; + state[1][0]=temp; + + // Rotate second row 2 columns to right + temp=state[2][0]; + state[2][0]=state[2][2]; + state[2][2]=temp; + + temp=state[2][1]; + state[2][1]=state[2][3]; + state[2][3]=temp; + + // Rotate third row 3 columns to right + temp=state[3][0]; + state[3][0]=state[3][1]; + state[3][1]=state[3][2]; + state[3][2]=state[3][3]; + state[3][3]=temp; +} + + +// Cipher is the main function that encrypts the PlainText. +static void Cipher() +{ + uint8_t i, j, round = 0; + + //Copy the input PlainText to state array. + for(i = 0; i < 4; ++i) + { + for(j = 0; j < 4 ; ++j) + { + state[j][i] = in[(i * 4) + j]; + } + } + + // Add the First round key to the state before starting the rounds. + AddRoundKey(0); + + // There will be Nr rounds. + // The first Nr-1 rounds are identical. + // These Nr-1 rounds are executed in the loop below. + for(round = 1; round < Nr; ++round) + { + SubBytes(); + ShiftRows(); + MixColumns(); + AddRoundKey(round); + } + + // The last round is given below. + // The MixColumns function is not here in the last round. + SubBytes(); + ShiftRows(); + AddRoundKey(Nr); + + // The encryption process is over. + // Copy the state array to output array. + for(i = 0; i < 4; ++i) + { + for(j = 0; j < 4; ++j) + { + out[(i * 4) + j] = state[j][i]; + } + } +} + +static void InvCipher() +{ + uint8_t i,j,round=0; + + //Copy the input CipherText to state array. + for(i=0;i<4;i++) + { + for(j=0;j<4;j++) + { + state[j][i] = in[i*4 + j]; + } + } + + // Add the First round key to the state before starting the rounds. + AddRoundKey(Nr); + + // There will be Nr rounds. + // The first Nr-1 rounds are identical. + // These Nr-1 rounds are executed in the loop below. + for(round=Nr-1;round>0;round--) + { + InvShiftRows(); + InvSubBytes(); + AddRoundKey(round); + InvMixColumns(); + } + + // The last round is given below. + // The MixColumns function is not here in the last round. + InvShiftRows(); + InvSubBytes(); + AddRoundKey(0); + + // The decryption process is over. + // Copy the state array to output array. + for(i=0;i<4;i++) + { + for(j=0;j<4;j++) + { + out[i*4+j]=state[j][i]; + } + } +} + + +/*****************************************************************************/ +/* Public functions: */ +/*****************************************************************************/ + +void AES128_ECB_encrypt(uint8_t* input, uint8_t* key, uint8_t *output) +{ + // Copy the Key and CipherText + Key = key; + in = input; + out = output; + + // The KeyExpansion routine must be called before encryption. + KeyExpansion(); + + // The next function call encrypts the PlainText with the Key using AES algorithm. + Cipher(); +} + +void AES128_ECB_decrypt(uint8_t* input, uint8_t* key, uint8_t *output) +{ + Key = key; + in = input; + out = output; + + KeyExpansion(); + + InvCipher(); +} + +#endif //_AES_C_ + + diff --git a/src/aes.h b/src/aes.h new file mode 100644 index 0000000..5fb2176 --- /dev/null +++ b/src/aes.h @@ -0,0 +1,16 @@ +#ifndef _AES_H_ +#define _AES_H_ + +#include <stdint.h> + +#ifdef __cplusplus +extern "C" { +#endif /* __cplusplus */ + +void AES128_ECB_encrypt(uint8_t* input, uint8_t* key, uint8_t *output); +void AES128_ECB_decrypt(uint8_t* input, uint8_t* key, uint8_t *output); + +#ifdef __cplusplus +} +#endif /* __cplusplus */ +#endif //_AES_H_
This code is inspired by JeanDo ostc-companion.
Signed-off-by: Anton Lundin glance@acc.umu.se --- src/hw_ostc3.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+)
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index b78702c..de1d865 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -21,6 +21,7 @@
#include <string.h> // memcmp, memcpy #include <stdlib.h> // malloc, free +#include <stdio.h> // FILE, fopen
#include <libdivecomputer/hw_ostc3.h>
@@ -30,6 +31,7 @@ #include "checksum.h" #include "ringbuffer.h" #include "array.h" +#include "aes.h"
#define ISINSTANCE(device) dc_device_isinstance((device), &hw_ostc3_device_vtable)
@@ -43,6 +45,7 @@ #define SZ_VERSION (SZ_CUSTOMTEXT + 4) #define SZ_MEMORY 0x200000 #define SZ_CONFIG 4 +#define SZ_FIRMWARE 0x01E000 // 120KB
#define RB_LOGBOOK_SIZE 256 #define RB_LOGBOOK_COUNT 256 @@ -622,3 +625,130 @@ hw_ostc3_device_config_reset (dc_device_t *abstract)
return DC_STATUS_SUCCESS; } + +typedef struct hw_ostc3_firmware_t { + unsigned char data[SZ_FIRMWARE]; + unsigned int checksum; +} hw_ostc3_firmware_t; + +// FIXME: This key is specific to the ostc3 +// The ostc sport has another key but the same protocoll +// How should we refactor the code for that? +static unsigned char ostc3_key[16] = { + 0xF1, 0xE9, 0xB0, 0x30, + 0x45, 0x6F, 0xBE, 0x55, + 0xFF, 0xE7, 0xF8, 0x31, + 0x13, 0x6C, 0xF2, 0xFE +}; + +static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t *firmware) +{ + unsigned short low = 0; + unsigned short high = 0; + for(unsigned int i = 0;i < SZ_FIRMWARE;i++) + { + low += firmware->data[i]; + high += low; + } + return (((unsigned int)high) << 16) + low; +} + + +static dc_status_t +hw_ostc3_firmware_readline(FILE *fp, unsigned int addr, char data[], unsigned int size) +{ + char ascii[40]; + // 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data. + unsigned line_size = size * 2 + 1 + 6 + 1; + unsigned char faddr_byte[3]; + unsigned int faddr = 0; + if (line_size > sizeof(ascii)) + return DC_STATUS_INVALIDARGS; + if (fread (ascii, sizeof(char), line_size, fp) != line_size) + return DC_STATUS_IO; + if (ascii[0] != ':') + return DC_STATUS_IO; + if (array_convert_hex2bin(ascii + 1, 6, faddr_byte, sizeof(faddr_byte)) != 0) { + return DC_STATUS_IO; + } + faddr = array_uint24_be (faddr_byte); + if (faddr != addr) + return DC_STATUS_IO; + if (array_convert_hex2bin (ascii + 1 + 6, size*2, data, size) != 0) + return DC_STATUS_IO; + + return DC_STATUS_SUCCESS; +} + + +static dc_status_t +hw_ostc3_firmware_readfile (hw_ostc3_firmware_t *firmware, dc_context_t *context, const char *filename) +{ + dc_status_t rc = DC_STATUS_SUCCESS; + FILE *fp; + unsigned char iv[16] = {0}; + unsigned char tmpbuf[16] = {0}; + unsigned char encrypted[16] = {0}; + unsigned int bytes = 0, addr = 0, faddr = 0; + char checksum[4]; + + if (firmware == NULL) { + ERROR (context, "Invalid arguments."); + return DC_STATUS_INVALIDARGS; + } + + // Initialize the buffers. + memset (firmware->data, 0xFF, sizeof (firmware->data)); + firmware->checksum = 0; + + fp = fopen (filename, "rb"); + if (fp == NULL) { + ERROR (context, "Failed to open the file."); + return DC_STATUS_IO; + } + + rc = hw_ostc3_firmware_readline(fp, 0, iv, sizeof(iv)); + if (rc != DC_STATUS_SUCCESS) { + fclose (fp); + ERROR (context, "Failed to parse header."); + return rc; + } + bytes += 16; + + // Load the iv for AES-FCB-mode + AES128_ECB_encrypt(iv, ostc3_key, tmpbuf); + + for(addr = 0; addr < SZ_FIRMWARE; addr += 16, bytes += 16) { + rc = hw_ostc3_firmware_readline(fp, bytes, encrypted, sizeof(encrypted)); + if (rc != DC_STATUS_SUCCESS) { + fclose (fp); + ERROR (context, "Failed to parse file data."); + return rc; + } + + // Decrypt AES-FCB data + for(int i=0;i < 16;i++) + firmware->data[addr + i] = encrypted[i] ^ tmpbuf[i]; + + // Run the next round of encryption + AES128_ECB_encrypt(encrypted, ostc3_key, tmpbuf); + } + + // This file format contains a tail with the checksum in + rc = hw_ostc3_firmware_readline(fp, bytes, checksum, sizeof(checksum)); + if (rc != DC_STATUS_SUCCESS) { + fclose (fp); + ERROR (context, "Failed to parse file tail."); + return rc; + } + fclose(fp); + + firmware->checksum = array_uint32_le(checksum); + + if (firmware->checksum != hw_ostc3_firmware_checksum(firmware)) { + ERROR (context, "Failed to verify file checksum."); + return DC_STATUS_IO; + } + + return DC_STATUS_SUCCESS; +}
On 21 November, 2014 - Anton Lundin wrote:
+// FIXME: This key is specific to the ostc3 +// The ostc sport has another key but the same protocoll +// How should we refactor the code for that? +static unsigned char ostc3_key[16] = {
- 0xF1, 0xE9, 0xB0, 0x30,
- 0x45, 0x6F, 0xBE, 0x55,
- 0xFF, 0xE7, 0xF8, 0x31,
- 0x13, 0x6C, 0xF2, 0xFE
+};
Took a new look at this and noticed that i was wrong. Its actually the HW Frog who got a different key and a slightly different protocol, and the OSTC Sport actually uses the same key and the same protocol.
So its actually just the comment here thats wrong.
//Anton
On 24 November, 2014 - Anton Lundin wrote:
On 21 November, 2014 - Anton Lundin wrote:
+// FIXME: This key is specific to the ostc3 +// The ostc sport has another key but the same protocoll +// How should we refactor the code for that? +static unsigned char ostc3_key[16] = {
- 0xF1, 0xE9, 0xB0, 0x30,
- 0x45, 0x6F, 0xBE, 0x55,
- 0xFF, 0xE7, 0xF8, 0x31,
- 0x13, 0x6C, 0xF2, 0xFE
+};
Took a new look at this and noticed that i was wrong. Its actually the HW Frog who got a different key and a slightly different protocol, and the OSTC Sport actually uses the same key and the same protocol.
So its actually just the comment here thats wrong.
And now I've actually gotten around to test this code against a OSTC Sport, and it works as is, YEY =)
//Anton
On 21-11-14 21:28, Anton Lundin wrote:
+// FIXME: This key is specific to the ostc3 +// The ostc sport has another key but the same protocoll +// How should we refactor the code for that? +static unsigned char ostc3_key[16] = {
- 0xF1, 0xE9, 0xB0, 0x30,
- 0x45, 0x6F, 0xBE, 0x55,
- 0xFF, 0xE7, 0xF8, 0x31,
- 0x13, 0x6C, 0xF2, 0xFE
+};
This should be marked as const.
+static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t *firmware) +{
- unsigned short low = 0;
- unsigned short high = 0;
- for(unsigned int i = 0;i < SZ_FIRMWARE;i++)
- {
low += firmware->data[i];high += low;- }
- return (((unsigned int)high) << 16) + low;
+}
This looks like a variant of the fletcher16 checksum (with a 16bit sum instead of 8bit sum, and modulo 2^n instead of 2^n-1). So I suggest moving it to checksum.c where it can be reused elsewhere.
+static dc_status_t +hw_ostc3_firmware_readline(FILE *fp, unsigned int addr, char data[], unsigned int size) +{
- char ascii[40];
- // 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data.
- unsigned line_size = size * 2 + 1 + 6 + 1;
- unsigned char faddr_byte[3];
- unsigned int faddr = 0;
- if (line_size > sizeof(ascii))
return DC_STATUS_INVALIDARGS;- if (fread (ascii, sizeof(char), line_size, fp) != line_size)
return DC_STATUS_IO;- if (ascii[0] != ':')
return DC_STATUS_IO;- if (array_convert_hex2bin(ascii + 1, 6, faddr_byte, sizeof(faddr_byte)) != 0) {
return DC_STATUS_IO;- }
- faddr = array_uint24_be (faddr_byte);
- if (faddr != addr)
return DC_STATUS_IO;- if (array_convert_hex2bin (ascii + 1 + 6, size*2, data, size) != 0)
return DC_STATUS_IO;- return DC_STATUS_SUCCESS;
+}
This will needs some improvements to be able to deal with different line endings (e.g CRLF vs LF). See ihex.c for an example.
DC_STATUS_DATAFORMAT is also a more appropriate error code than DC_STATUS_IO here. Except for the fread one.
+static dc_status_t +hw_ostc3_firmware_readfile (hw_ostc3_firmware_t *firmware, dc_context_t *context, const char *filename) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- FILE *fp;
- unsigned char iv[16] = {0};
- unsigned char tmpbuf[16] = {0};
- unsigned char encrypted[16] = {0};
- unsigned int bytes = 0, addr = 0, faddr = 0;
- char checksum[4];
- if (firmware == NULL) {
ERROR (context, "Invalid arguments.");return DC_STATUS_INVALIDARGS;- }
- // Initialize the buffers.
- memset (firmware->data, 0xFF, sizeof (firmware->data));
- firmware->checksum = 0;
- fp = fopen (filename, "rb");
- if (fp == NULL) {
ERROR (context, "Failed to open the file.");return DC_STATUS_IO;- }
- rc = hw_ostc3_firmware_readline(fp, 0, iv, sizeof(iv));
- if (rc != DC_STATUS_SUCCESS) {
fclose (fp);ERROR (context, "Failed to parse header.");return rc;- }
- bytes += 16;
- // Load the iv for AES-FCB-mode
- AES128_ECB_encrypt(iv, ostc3_key, tmpbuf);
- for(addr = 0; addr < SZ_FIRMWARE; addr += 16, bytes += 16) {
rc = hw_ostc3_firmware_readline(fp, bytes, encrypted, sizeof(encrypted));if (rc != DC_STATUS_SUCCESS) {fclose (fp);ERROR (context, "Failed to parse file data.");return rc;}// Decrypt AES-FCB datafor(int i=0;i < 16;i++)firmware->data[addr + i] = encrypted[i] ^ tmpbuf[i];// Run the next round of encryptionAES128_ECB_encrypt(encrypted, ostc3_key, tmpbuf);- }
- // This file format contains a tail with the checksum in
- rc = hw_ostc3_firmware_readline(fp, bytes, checksum, sizeof(checksum));
- if (rc != DC_STATUS_SUCCESS) {
fclose (fp);ERROR (context, "Failed to parse file tail.");return rc;- }
- fclose(fp);
- firmware->checksum = array_uint32_le(checksum);
- if (firmware->checksum != hw_ostc3_firmware_checksum(firmware)) {
ERROR (context, "Failed to verify file checksum.");return DC_STATUS_IO;- }
- return DC_STATUS_SUCCESS;
+}
Two questions here:
If I understand correctly, the address in the firmware file is not really an address in the normal sense, but more like a sequence number, right? Can they be in a random order, or are they always nicely incrementing as the code assumes?
Is the payload always guaranteed to be 16 bytes (except for the last line with the checksum)? I assume it is because there is no length stored anywhere.
Jef
On 15 December, 2014 - Jef Driesen wrote:
On 21-11-14 21:28, Anton Lundin wrote:
+// FIXME: This key is specific to the ostc3 +// The ostc sport has another key but the same protocoll +// How should we refactor the code for that? +static unsigned char ostc3_key[16] = {
- 0xF1, 0xE9, 0xB0, 0x30,
- 0x45, 0x6F, 0xBE, 0x55,
- 0xFF, 0xE7, 0xF8, 0x31,
- 0x13, 0x6C, 0xF2, 0xFE
+};
This should be marked as const.
Agreed and fixed.
+static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t *firmware) +{
- unsigned short low = 0;
- unsigned short high = 0;
- for(unsigned int i = 0;i < SZ_FIRMWARE;i++)
- {
low += firmware->data[i];high += low;- }
- return (((unsigned int)high) << 16) + low;
+}
This looks like a variant of the fletcher16 checksum (with a 16bit sum instead of 8bit sum, and modulo 2^n instead of 2^n-1). So I suggest moving it to checksum.c where it can be reused elsewhere.
Is that such a wise idea? Sure, its a cousin of fletcher16, but do you think anyone else will be using the same cousin of fletcher16?
If it where a straight fletcher16, i would have agreed with you but this one, I'm not so sure. Just think, what should we name the darn thing when we move it to checksum.c? ostc3_firmware_fletcher16_almost()?
+static dc_status_t +hw_ostc3_firmware_readline(FILE *fp, unsigned int addr, char data[], unsigned int size) +{
- char ascii[40];
- // 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data.
- unsigned line_size = size * 2 + 1 + 6 + 1;
- unsigned char faddr_byte[3];
- unsigned int faddr = 0;
- if (line_size > sizeof(ascii))
return DC_STATUS_INVALIDARGS;- if (fread (ascii, sizeof(char), line_size, fp) != line_size)
return DC_STATUS_IO;- if (ascii[0] != ':')
return DC_STATUS_IO;- if (array_convert_hex2bin(ascii + 1, 6, faddr_byte, sizeof(faddr_byte)) != 0) {
return DC_STATUS_IO;- }
- faddr = array_uint24_be (faddr_byte);
- if (faddr != addr)
return DC_STATUS_IO;- if (array_convert_hex2bin (ascii + 1 + 6, size*2, data, size) != 0)
return DC_STATUS_IO;- return DC_STATUS_SUCCESS;
+}
This will needs some improvements to be able to deal with different line endings (e.g CRLF vs LF). See ihex.c for an example.
None of the Ostc3 or Ostc Sport firmware files have CRLF endings. Probably due to the tool with which they bin generated.
But, when re-reading the HexFile::loadEncrypted of ostc-planner, it would accept a file with CRLF encodings, so we should do the same.
Fixed.
DC_STATUS_DATAFORMAT is also a more appropriate error code than DC_STATUS_IO here. Except for the fread one.
Agreed and fixed.
+static dc_status_t +hw_ostc3_firmware_readfile (hw_ostc3_firmware_t *firmware, dc_context_t *context, const char *filename) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- FILE *fp;
- unsigned char iv[16] = {0};
- unsigned char tmpbuf[16] = {0};
- unsigned char encrypted[16] = {0};
- unsigned int bytes = 0, addr = 0, faddr = 0;
- char checksum[4];
- if (firmware == NULL) {
ERROR (context, "Invalid arguments.");return DC_STATUS_INVALIDARGS;- }
- // Initialize the buffers.
- memset (firmware->data, 0xFF, sizeof (firmware->data));
- firmware->checksum = 0;
- fp = fopen (filename, "rb");
- if (fp == NULL) {
ERROR (context, "Failed to open the file.");return DC_STATUS_IO;- }
- rc = hw_ostc3_firmware_readline(fp, 0, iv, sizeof(iv));
- if (rc != DC_STATUS_SUCCESS) {
fclose (fp);ERROR (context, "Failed to parse header.");return rc;- }
- bytes += 16;
- // Load the iv for AES-FCB-mode
- AES128_ECB_encrypt(iv, ostc3_key, tmpbuf);
- for(addr = 0; addr < SZ_FIRMWARE; addr += 16, bytes += 16) {
rc = hw_ostc3_firmware_readline(fp, bytes, encrypted, sizeof(encrypted));if (rc != DC_STATUS_SUCCESS) {fclose (fp);ERROR (context, "Failed to parse file data.");return rc;}// Decrypt AES-FCB datafor(int i=0;i < 16;i++)firmware->data[addr + i] = encrypted[i] ^ tmpbuf[i];// Run the next round of encryptionAES128_ECB_encrypt(encrypted, ostc3_key, tmpbuf);- }
- // This file format contains a tail with the checksum in
- rc = hw_ostc3_firmware_readline(fp, bytes, checksum, sizeof(checksum));
- if (rc != DC_STATUS_SUCCESS) {
fclose (fp);ERROR (context, "Failed to parse file tail.");return rc;- }
- fclose(fp);
- firmware->checksum = array_uint32_le(checksum);
- if (firmware->checksum != hw_ostc3_firmware_checksum(firmware)) {
ERROR (context, "Failed to verify file checksum.");return DC_STATUS_IO;- }
- return DC_STATUS_SUCCESS;
+}
Two questions here:
If I understand correctly, the address in the firmware file is not really an address in the normal sense, but more like a sequence number, right? Can they be in a random order, or are they always nicely incrementing as the code assumes?
All the firmware files I've seen, they are always linear. The ostc-planner code also assumes that they always will be and will fail reading the file if they aren't, so thats the reason this code will reject such a file to.
Is the payload always guaranteed to be 16 bytes (except for the last line with the checksum)? I assume it is because there is no length stored anywhere.
I haven't seen any files in any other format. They are all the exact same size, 307256.
If yo take a look at the decrypted data, its padded to be this exact firmware size.
//Anton
On 15-12-14 23:28, Anton Lundin wrote:
On 15 December, 2014 - Jef Driesen wrote:
+static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t *firmware) +{
- unsigned short low = 0;
- unsigned short high = 0;
- for(unsigned int i = 0;i < SZ_FIRMWARE;i++)
- {
low += firmware->data[i];high += low;- }
- return (((unsigned int)high) << 16) + low;
+}
This looks like a variant of the fletcher16 checksum (with a 16bit sum instead of 8bit sum, and modulo 2^n instead of 2^n-1). So I suggest moving it to checksum.c where it can be reused elsewhere.
Is that such a wise idea? Sure, its a cousin of fletcher16, but do you think anyone else will be using the same cousin of fletcher16?
If it where a straight fletcher16, i would have agreed with you but this one, I'm not so sure. Just think, what should we name the darn thing when we move it to checksum.c? ostc3_firmware_fletcher16_almost()?
Well, it's not really a must for me. Having all checksums in a single file is just convenient when trying to reverse engineer some unknown checksum. I can simply try all checksums in that file, without having to remember whether there are any others elsewhere. That's all.
The checksum.c file already contains the custom checksum_add_uint4 that's only used in a single place. The CCITT is also one specific variant of the CRC algorithm.
For the name we could pick checksum_fletcher_uint32, or something like that.
But as I said, it's not really a must.
+static dc_status_t +hw_ostc3_firmware_readline(FILE *fp, unsigned int addr, char data[], unsigned int size) +{
- char ascii[40];
- // 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data.
- unsigned line_size = size * 2 + 1 + 6 + 1;
- unsigned char faddr_byte[3];
- unsigned int faddr = 0;
- if (line_size > sizeof(ascii))
return DC_STATUS_INVALIDARGS;- if (fread (ascii, sizeof(char), line_size, fp) != line_size)
return DC_STATUS_IO;- if (ascii[0] != ':')
return DC_STATUS_IO;- if (array_convert_hex2bin(ascii + 1, 6, faddr_byte, sizeof(faddr_byte)) != 0) {
return DC_STATUS_IO;- }
- faddr = array_uint24_be (faddr_byte);
- if (faddr != addr)
return DC_STATUS_IO;- if (array_convert_hex2bin (ascii + 1 + 6, size*2, data, size) != 0)
return DC_STATUS_IO;- return DC_STATUS_SUCCESS;
+}
This will needs some improvements to be able to deal with different line endings (e.g CRLF vs LF). See ihex.c for an example.
None of the Ostc3 or Ostc Sport firmware files have CRLF endings. Probably due to the tool with which they bin generated.
But, when re-reading the HexFile::loadEncrypted of ostc-planner, it would accept a file with CRLF encodings, so we should do the same.
Fixed.
Even if the files are generated with one particular line ending, we can't really guarantee that the original line ending is preserved when those files are send as text files over email, ftp, etc. So I think it's a nice feature to be able to handle this transparently. Especially because it's not very difficult to do. After all, it might save people some frustration.
Two questions here:
If I understand correctly, the address in the firmware file is not really an address in the normal sense, but more like a sequence number, right? Can they be in a random order, or are they always nicely incrementing as the code assumes?
All the firmware files I've seen, they are always linear. The ostc-planner code also assumes that they always will be and will fail reading the file if they aren't, so thats the reason this code will reject such a file to.
Is the payload always guaranteed to be 16 bytes (except for the last line with the checksum)? I assume it is because there is no length stored anywhere.
I haven't seen any files in any other format. They are all the exact same size, 307256.
If yo take a look at the decrypted data, its padded to be this exact firmware size.
I just wanted to double check that we don't start with wrong assumptions.
Jef
This lifts the OSTC3 INIT command out from the open function and does that separately. This is refactoring to be able to enter service mode so we can access service mode commands.
Signed-off-by: Anton Lundin glance@acc.umu.se --- src/hw_ostc3.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 92 insertions(+), 18 deletions(-)
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index de1d865..7fa38f8 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -67,6 +67,11 @@ typedef struct hw_ostc3_device_t { dc_device_t base; serial_t *port; unsigned char fingerprint[5]; + enum hw_ostc3_state { + NONE, + OPEN, + DOWNLOAD, + } state; } hw_ostc3_device_t;
static dc_status_t hw_ostc3_device_set_fingerprint (dc_device_t *abstract, const unsigned char data[], unsigned int size); @@ -220,6 +225,7 @@ hw_ostc3_device_open (dc_device_t **out, dc_context_t *context, const char *name // Set the default values. device->port = NULL; memset (device->fingerprint, 0, sizeof (device->fingerprint)); + device->state = NONE;
// Open the device. int rc = serial_open (&device->port, context, name); @@ -249,18 +255,48 @@ hw_ostc3_device_open (dc_device_t **out, dc_context_t *context, const char *name // Make sure everything is in a sane state. serial_sleep (device->port, 300); serial_flush (device->port, SERIAL_QUEUE_BOTH); + device->state = OPEN; + + *out = (dc_device_t *) device; + + return DC_STATUS_SUCCESS; +} + + +static dc_status_t +hw_ostc3_device_init (dc_device_t *abstract) +{ + hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract; + dc_context_t *context = (abstract ? abstract->context : NULL); + + if (device->state != OPEN) + return DC_STATUS_INVALIDARGS;
// Send the init command. dc_status_t status = hw_ostc3_transfer (device, NULL, INIT, NULL, 0, NULL, 0); if (status != DC_STATUS_SUCCESS) { ERROR (context, "Failed to send the command."); - serial_close (device->port); - free (device); return status; }
- *out = (dc_device_t *) device; + device->state = DOWNLOAD; + + return DC_STATUS_SUCCESS; +} +
+static dc_status_t +hw_ostc3_check_state_or_init(dc_device_t *abstract) +{ + hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract; + dc_status_t rc; + + if (device->state == OPEN) { + if ((rc = hw_ostc3_device_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + } else if (device->state != DOWNLOAD) { + return DC_STATUS_INVALIDARGS; + } return DC_STATUS_SUCCESS; }
@@ -269,14 +305,20 @@ static dc_status_t hw_ostc3_device_close (dc_device_t *abstract) { hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract; + dc_status_t status;
- // Send the exit command. - dc_status_t status = hw_ostc3_transfer (device, NULL, EXIT, NULL, 0, NULL, 0); - if (status != DC_STATUS_SUCCESS) { - ERROR (abstract->context, "Failed to send the command."); - serial_close (device->port); - free (device); - return status; + if (device->state == NONE) + return DC_STATUS_INVALIDARGS; + + // Send the exit command + if (device->state != OPEN) { + status = hw_ostc3_transfer (device, NULL, EXIT, NULL, 0, NULL, 0); + if (status != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to send the command."); + serial_close (device->port); + free (device); + return status; + } }
// Close the device. @@ -313,6 +355,7 @@ dc_status_t hw_ostc3_device_version (dc_device_t *abstract, unsigned char data[], unsigned int size) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_status_t rc;
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS; @@ -320,8 +363,11 @@ hw_ostc3_device_version (dc_device_t *abstract, unsigned char data[], unsigned i if (size != SZ_VERSION) return DC_STATUS_INVALIDARGS;
+ if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + // Send the command. - dc_status_t rc = hw_ostc3_transfer (device, NULL, IDENTITY, NULL, 0, data, size); + rc = hw_ostc3_transfer (device, NULL, IDENTITY, NULL, 0, data, size); if (rc != DC_STATUS_SUCCESS) return rc;
@@ -333,15 +379,19 @@ static dc_status_t hw_ostc3_device_foreach (dc_device_t *abstract, dc_dive_callback_t callback, void *userdata) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_status_t rc;
// Enable progress notifications. dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; progress.maximum = (RB_LOGBOOK_SIZE * RB_LOGBOOK_COUNT) + SZ_MEMORY; device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
+ if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + // Download the version data. unsigned char id[SZ_VERSION] = {0}; - dc_status_t rc = hw_ostc3_device_version (abstract, id, sizeof (id)); + rc = hw_ostc3_device_version (abstract, id, sizeof (id)); if (rc != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to read the version."); return rc; @@ -497,6 +547,7 @@ dc_status_t hw_ostc3_device_clock (dc_device_t *abstract, const dc_datetime_t *datetime) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_status_t rc;
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS; @@ -506,11 +557,14 @@ hw_ostc3_device_clock (dc_device_t *abstract, const dc_datetime_t *datetime) return DC_STATUS_INVALIDARGS; }
+ if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + // Send the command. unsigned char packet[6] = { datetime->hour, datetime->minute, datetime->second, datetime->month, datetime->day, datetime->year - 2000}; - dc_status_t rc = hw_ostc3_transfer (device, NULL, CLOCK, packet, sizeof (packet), NULL, 0); + rc = hw_ostc3_transfer (device, NULL, CLOCK, packet, sizeof (packet), NULL, 0); if (rc != DC_STATUS_SUCCESS) return rc;
@@ -522,10 +576,14 @@ dc_status_t hw_ostc3_device_display (dc_device_t *abstract, const char *text) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_status_t rc;
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS;
+ if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + // Pad the data packet with spaces. unsigned char packet[SZ_DISPLAY] = {0}; if (hw_ostc3_strncpy (packet, sizeof (packet), text) != 0) { @@ -534,7 +592,7 @@ hw_ostc3_device_display (dc_device_t *abstract, const char *text) }
// Send the command. - dc_status_t rc = hw_ostc3_transfer (device, NULL, DISPLAY, packet, sizeof (packet), NULL, 0); + rc = hw_ostc3_transfer (device, NULL, DISPLAY, packet, sizeof (packet), NULL, 0); if (rc != DC_STATUS_SUCCESS) return rc;
@@ -546,10 +604,14 @@ dc_status_t hw_ostc3_device_customtext (dc_device_t *abstract, const char *text) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_status_t rc;
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS;
+ if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + // Pad the data packet with spaces. unsigned char packet[SZ_CUSTOMTEXT] = {0}; if (hw_ostc3_strncpy (packet, sizeof (packet), text) != 0) { @@ -558,7 +620,7 @@ hw_ostc3_device_customtext (dc_device_t *abstract, const char *text) }
// Send the command. - dc_status_t rc = hw_ostc3_transfer (device, NULL, CUSTOMTEXT, packet, sizeof (packet), NULL, 0); + rc = hw_ostc3_transfer (device, NULL, CUSTOMTEXT, packet, sizeof (packet), NULL, 0); if (rc != DC_STATUS_SUCCESS) return rc;
@@ -569,6 +631,7 @@ dc_status_t hw_ostc3_device_config_read (dc_device_t *abstract, unsigned int config, unsigned char data[], unsigned int size) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_status_t rc;
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS; @@ -578,9 +641,12 @@ hw_ostc3_device_config_read (dc_device_t *abstract, unsigned int config, unsigne return DC_STATUS_INVALIDARGS; }
+ if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + // Send the command. unsigned char command[1] = {config}; - dc_status_t rc = hw_ostc3_transfer (device, NULL, READ, command, sizeof (command), data, size); + rc = hw_ostc3_transfer (device, NULL, READ, command, sizeof (command), data, size); if (rc != DC_STATUS_SUCCESS) return rc;
@@ -591,6 +657,7 @@ dc_status_t hw_ostc3_device_config_write (dc_device_t *abstract, unsigned int config, const unsigned char data[], unsigned int size) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_status_t rc;
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS; @@ -600,10 +667,13 @@ hw_ostc3_device_config_write (dc_device_t *abstract, unsigned int config, const return DC_STATUS_INVALIDARGS; }
+ if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + // Send the command. unsigned char command[SZ_CONFIG + 1] = {config}; memcpy(command + 1, data, size); - dc_status_t rc = hw_ostc3_transfer (device, NULL, WRITE, command, size + 1, NULL, 0); + rc = hw_ostc3_transfer (device, NULL, WRITE, command, size + 1, NULL, 0); if (rc != DC_STATUS_SUCCESS) return rc;
@@ -614,12 +684,16 @@ dc_status_t hw_ostc3_device_config_reset (dc_device_t *abstract) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_status_t rc;
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS;
+ if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS) + return rc; + // Send the command. - dc_status_t rc = hw_ostc3_transfer (device, NULL, RESET, NULL, 0, NULL, 0); + rc = hw_ostc3_transfer (device, NULL, RESET, NULL, 0, NULL, 0); if (rc != DC_STATUS_SUCCESS) return rc;
On 21-11-14 21:28, Anton Lundin wrote:
--- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -67,6 +67,11 @@ typedef struct hw_ostc3_device_t { dc_device_t base; serial_t *port; unsigned char fingerprint[5];
- enum hw_ostc3_state {
NONE,OPEN,DOWNLOAD,- } state; } hw_ostc3_device_t;
Move the enum outside of the struct and give it a proper typedef. (This is mainly for the msvc C++ compiler. When defined inside the struct you need to prefix the values with the C++ hw_ostc3_device_t:: syntax. And that doesn't work in C mode.)
static dc_status_t hw_ostc3_device_set_fingerprint (dc_device_t *abstract, const unsigned char data[], unsigned int size); @@ -220,6 +225,7 @@ hw_ostc3_device_open (dc_device_t **out, dc_context_t *context, const char *name // Set the default values. device->port = NULL; memset (device->fingerprint, 0, sizeof (device->fingerprint));
device->state = NONE;
// Open the device. int rc = serial_open (&device->port, context, name);
@@ -249,18 +255,48 @@ hw_ostc3_device_open (dc_device_t **out, dc_context_t *context, const char *name // Make sure everything is in a sane state. serial_sleep (device->port, 300); serial_flush (device->port, SERIAL_QUEUE_BOTH);
- device->state = OPEN;
- *out = (dc_device_t *) device;
- return DC_STATUS_SUCCESS;
+}
Why do we need both NONE and OPEN? I don't see any difference between the two. The api prevents you from returning an handle in the NONE state. So I see no reason to keep the OPEN state.
+static dc_status_t +hw_ostc3_device_init (dc_device_t *abstract) +{
- hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
- if (device->state != OPEN)
return DC_STATUS_INVALIDARGS;
This condition can never happen. (see previous comment)
@@ -269,14 +305,20 @@ static dc_status_t hw_ostc3_device_close (dc_device_t *abstract) { hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract;
- dc_status_t status;
- // Send the exit command.
- dc_status_t status = hw_ostc3_transfer (device, NULL, EXIT, NULL, 0, NULL, 0);
- if (status != DC_STATUS_SUCCESS) {
ERROR (abstract->context, "Failed to send the command.");serial_close (device->port);free (device);return status;
- if (device->state == NONE)
return DC_STATUS_INVALIDARGS;
Same here. If it where possible, you even have a memory leak here. The close function should always clean up all resources. If the close fails, how are you supposed to clean up? The return value is purely informational.
// Close the device. @@ -313,6 +355,7 @@ dc_status_t hw_ostc3_device_version (dc_device_t *abstract, unsigned char data[], unsigned int size) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_status_t rc;
Initialize variables as much as possible.
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS; @@ -320,8 +363,11 @@ hw_ostc3_device_version (dc_device_t *abstract, unsigned char data[], unsigned i if (size != SZ_VERSION) return DC_STATUS_INVALIDARGS;
- if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS)
return rc;
Move the assignment out of the if condition. That makes the code more readable.
On 15 December, 2014 - Jef Driesen wrote:
On 21-11-14 21:28, Anton Lundin wrote:
--- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -67,6 +67,11 @@ typedef struct hw_ostc3_device_t { dc_device_t base; serial_t *port; unsigned char fingerprint[5];
- enum hw_ostc3_state {
NONE,OPEN,DOWNLOAD,- } state;
} hw_ostc3_device_t;
Move the enum outside of the struct and give it a proper typedef. (This is mainly for the msvc C++ compiler. When defined inside the struct you need to prefix the values with the C++ hw_ostc3_device_t:: syntax. And that doesn't work in C mode.)
Zomg, but i hear you. Fixing.
static dc_status_t hw_ostc3_device_set_fingerprint (dc_device_t *abstract, const unsigned char data[], unsigned int size); @@ -220,6 +225,7 @@ hw_ostc3_device_open (dc_device_t **out, dc_context_t *context, const char *name // Set the default values. device->port = NULL; memset (device->fingerprint, 0, sizeof (device->fingerprint));
device->state = NONE;
// Open the device. int rc = serial_open (&device->port, context, name);
@@ -249,18 +255,48 @@ hw_ostc3_device_open (dc_device_t **out, dc_context_t *context, const char *name // Make sure everything is in a sane state. serial_sleep (device->port, 300); serial_flush (device->port, SERIAL_QUEUE_BOTH);
- device->state = OPEN;
- *out = (dc_device_t *) device;
- return DC_STATUS_SUCCESS;
+}
Why do we need both NONE and OPEN? I don't see any difference between the two. The api prevents you from returning an handle in the NONE state. So I see no reason to keep the OPEN state.
I just thought it would be sane to actually represent all the states we can have, if we where to change something in the future in how we do things.
+static dc_status_t +hw_ostc3_device_init (dc_device_t *abstract) +{
- hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
- if (device->state != OPEN)
return DC_STATUS_INVALIDARGS;This condition can never happen. (see previous comment)
This might be a bit over cautious, Fixed.
@@ -269,14 +305,20 @@ static dc_status_t hw_ostc3_device_close (dc_device_t *abstract) { hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract;
- dc_status_t status;
- // Send the exit command.
- dc_status_t status = hw_ostc3_transfer (device, NULL, EXIT, NULL, 0, NULL, 0);
- if (status != DC_STATUS_SUCCESS) {
ERROR (abstract->context, "Failed to send the command.");serial_close (device->port);free (device);return status;
- if (device->state == NONE)
return DC_STATUS_INVALIDARGS;Same here. If it where possible, you even have a memory leak here. The close function should always clean up all resources. If the close fails, how are you supposed to clean up? The return value is purely informational.
Makes sense. Fixed.
// Close the device. @@ -313,6 +355,7 @@ dc_status_t hw_ostc3_device_version (dc_device_t *abstract, unsigned char data[], unsigned int size) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_status_t rc;
Initialize variables as much as possible.
Fixed.
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS; @@ -320,8 +363,11 @@ hw_ostc3_device_version (dc_device_t *abstract, unsigned char data[], unsigned i if (size != SZ_VERSION) return DC_STATUS_INVALIDARGS;
- if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS)
return rc;Move the assignment out of the if condition. That makes the code more readable.
Fixed.
//Anton
On 15-12-14 23:43, Anton Lundin wrote:
On 15 December, 2014 - Jef Driesen wrote:
Why do we need both NONE and OPEN? I don't see any difference between the two. The api prevents you from returning an handle in the NONE state. So I see no reason to keep the OPEN state.
I just thought it would be sane to actually represent all the states we can have, if we where to change something in the future in how we do things.
Sure, but my point is that your NONE state is not really a valid state. It only exists for a very brief moment in the open function. If it succeeds the final state is OPEN, and if it fails we don't return a valid device handle.
With all the other patches in mind, we basically need 4 different states:
* NONE: This is the initial state after opening the connection. At this point we the serial connection is opened, but the device hasn't been initialized yet. From here you can go into DOWNLOAD or SERVICE mode.
* DOWNLOAD: The normal download mode.
* SERVICE: The state during firmware upgrades. You can only get stuck in this state if a firmware upgrade fails. If it succeeds, you automatically go to the next state.
* REBOOTING: This is the final state after a successful firmware update. In this state no other communication is possible and the caller should close the device handle.
Maybe it's more intuitive to rename NONE to OPEN and REBOOTING to CLOSED?
BTW, do you know whether there is a limitation on which commands can be send in download or service mode? Can you use for example the IDENTITY, CLOCK, CUSTOMTEXT, READ, WRITE, RESET in service mode, or are they restricted to download mode only? This matters because these commands have corresponding public functions that can be called by the user.
The remaining comments I have are mostly purely cosmetic things. Nothing critical, just nice to have. For example the new functions are named with two different prefixes: hw_ostc3_firmware_xxx and hw_ostc3_device_xxx. It would be nice to make this more consistent. For example:
hw_ostc3_device_init -> hw_ostc3_device_init_download hw_ostc3_device_service_mode -> hw_ostc3_device_init_service hw_ostc3_device_erase_range -> hw_ostc3_firmware_erase hw_ostc3_device_read_block -> hw_ostc3_firmware_block_read hw_ostc3_device_write_block -> hw_ostc3_firmware_block_write hw_ostc3_device_upgrade_firmware -> hw_ostc3_firmware_upgrade
And maybe also prefix the firmware commands with S_ to distinguish them from the normal ones. For example:
S_BLOCK_READ S_BLOCK_WRITE S_ERASE S_READY S_UPGRADE
If you don't mind, also move the hw_ostc3_firmware_t struct and ostc3_key near the top of the file.
Other than that, I think you have addressed all my comments and questions. So we're almost there. Can you submit a new patch series once you have put everything together?
Jef
On 17 December, 2014 - Jef Driesen wrote:
On 15-12-14 23:43, Anton Lundin wrote:
On 15 December, 2014 - Jef Driesen wrote:
Why do we need both NONE and OPEN? I don't see any difference between the two. The api prevents you from returning an handle in the NONE state. So I see no reason to keep the OPEN state.
I just thought it would be sane to actually represent all the states we can have, if we where to change something in the future in how we do things.
Sure, but my point is that your NONE state is not really a valid state. It only exists for a very brief moment in the open function. If it succeeds the final state is OPEN, and if it fails we don't return a valid device handle.
With all the other patches in mind, we basically need 4 different states:
- NONE: This is the initial state after opening the connection. At this
point we the serial connection is opened, but the device hasn't been initialized yet. From here you can go into DOWNLOAD or SERVICE mode.
DOWNLOAD: The normal download mode.
SERVICE: The state during firmware upgrades. You can only get stuck in
this state if a firmware upgrade fails. If it succeeds, you automatically go to the next state.
- REBOOTING: This is the final state after a successful firmware update. In
this state no other communication is possible and the caller should close the device handle.
Maybe it's more intuitive to rename NONE to OPEN and REBOOTING to CLOSED?
I'd say REBOOTING is better than CLOSED, because CLOSED might be confusing because the file descriptor is still open, and the only viable action is to close the device.
BTW, do you know whether there is a limitation on which commands can be send in download or service mode? Can you use for example the IDENTITY, CLOCK, CUSTOMTEXT, READ, WRITE, RESET in service mode, or are they restricted to download mode only? This matters because these commands have corresponding public functions that can be called by the user.
Yes, all those commands can be issued while in service mode to, but none of the service mode commands can be used in download mode.
The remaining comments I have are mostly purely cosmetic things. Nothing critical, just nice to have. For example the new functions are named with two different prefixes: hw_ostc3_firmware_xxx and hw_ostc3_device_xxx. It would be nice to make this more consistent. For example:
hw_ostc3_device_init -> hw_ostc3_device_init_download hw_ostc3_device_service_mode -> hw_ostc3_device_init_service hw_ostc3_device_erase_range -> hw_ostc3_firmware_erase hw_ostc3_device_read_block -> hw_ostc3_firmware_block_read hw_ostc3_device_write_block -> hw_ostc3_firmware_block_write hw_ostc3_device_upgrade_firmware -> hw_ostc3_firmware_upgrade
And maybe also prefix the firmware commands with S_ to distinguish them from the normal ones. For example:
S_BLOCK_READ S_BLOCK_WRITE S_ERASE S_READY S_UPGRADE
If you don't mind, also move the hw_ostc3_firmware_t struct and ostc3_key near the top of the file.
Other than that, I think you have addressed all my comments and questions. So we're almost there. Can you submit a new patch series once you have put everything together?
Yepp. Thats the plan when everything baked for this round of review.
I'll add a reviewed-by tag the patches to.
//Anton
On 2014-12-17 22:33, Anton Lundin wrote:
On 17 December, 2014 - Jef Driesen wrote:
Maybe it's more intuitive to rename NONE to OPEN and REBOOTING to CLOSED?
I'd say REBOOTING is better than CLOSED, because CLOSED might be confusing because the file descriptor is still open, and the only viable action is to close the device.
The state is not about the file descriptor, but I get your point. I'll keep REBOOTING. Maybe TERMINATED is even a more descriptive name than REBOOTING?
Jef
On 19 December, 2014 - Jef Driesen wrote:
On 2014-12-17 22:33, Anton Lundin wrote:
On 17 December, 2014 - Jef Driesen wrote:
Maybe it's more intuitive to rename NONE to OPEN and REBOOTING to CLOSED?
I'd say REBOOTING is better than CLOSED, because CLOSED might be confusing because the file descriptor is still open, and the only viable action is to close the device.
The state is not about the file descriptor, but I get your point. I'll keep REBOOTING. Maybe TERMINATED is even a more descriptive name than REBOOTING?
I thing REBOOTING is a better state name, because thats what came up in my head, but I'm fine with TERMINATED to.
//Anton
This code is inspired by JeanDo ostc-companion.
Signed-off-by: Anton Lundin glance@acc.umu.se --- src/hw_ostc3.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-)
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index 7fa38f8..4d6bf83 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -50,6 +50,7 @@ #define RB_LOGBOOK_SIZE 256 #define RB_LOGBOOK_COUNT 256
+#define S_READY 0x4C #define READY 0x4D #define HEADER 0x61 #define CLOCK 0x62 @@ -71,6 +72,7 @@ typedef struct hw_ostc3_device_t { NONE, OPEN, DOWNLOAD, + SERVICE, } state; } hw_ostc3_device_t;
@@ -196,7 +198,7 @@ hw_ostc3_transfer (hw_ostc3_device_t *device, }
// Verify the ready byte. - if (ready[0] != READY) { + if (!(ready[0] == READY || ready[0] == S_READY)) { ERROR (abstract->context, "Unexpected ready byte."); return DC_STATUS_PROTOCOL; } @@ -294,7 +296,7 @@ hw_ostc3_check_state_or_init(dc_device_t *abstract) if (device->state == OPEN) { if ((rc = hw_ostc3_device_init(abstract)) != DC_STATUS_SUCCESS) return rc; - } else if (device->state != DOWNLOAD) { + } else if (device->state != DOWNLOAD && device->state != SERVICE) { return DC_STATUS_INVALIDARGS; } return DC_STATUS_SUCCESS; @@ -826,3 +828,46 @@ hw_ostc3_firmware_readfile (hw_ostc3_firmware_t *firmware, dc_context_t *context
return DC_STATUS_SUCCESS; } + + +static dc_status_t +hw_ostc3_device_service_mode (dc_device_t *abstract) +{ + hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + unsigned char command[] = {0xAA, 0xAB, 0xCD, 0xEF}; + unsigned char output[5]; + dc_status_t rc; + int n; + + if (device->state != OPEN) + return DC_STATUS_INVALIDARGS; + + // We cant use hw_ostc3_transfer here, due to the different echos + n = serial_write (device->port, command, sizeof (command)); + if (n != sizeof (command)) { + ERROR (abstract->context, "Failed to send the command."); + return EXITCODE (n); + } + + // Give the device some time to enter service mode + serial_sleep(device->port, 100); + + // Read the response + n = serial_read (device->port, output, sizeof (output)); + if (n != sizeof (output)) { + ERROR (abstract->context, "Failed to receive the echo."); + return EXITCODE (n); + } + + // Verify the response to service mode + if (output[0] != 0x4B || output[1] != 0xAB || + output[2] != 0xCD || output[3] != 0xEF || + output[4] != S_READY) { + ERROR (abstract->context, "Failed to verify echo."); + return DC_STATUS_IO; + } + + device->state = SERVICE; + + return DC_STATUS_SUCCESS; +}
On 21-11-14 21:28, Anton Lundin wrote:
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index 7fa38f8..4d6bf83 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -50,6 +50,7 @@ #define RB_LOGBOOK_SIZE 256 #define RB_LOGBOOK_COUNT 256
+#define S_READY 0x4C #define READY 0x4D #define HEADER 0x61 #define CLOCK 0x62 @@ -71,6 +72,7 @@ typedef struct hw_ostc3_device_t { NONE, OPEN, DOWNLOAD,
SERVICE,@@ -196,7 +198,7 @@ hw_ostc3_transfer (hw_ostc3_device_t *device, }
// Verify the ready byte.
if (ready[0] != READY) {
if (!(ready[0] == READY || ready[0] == S_READY)) { ERROR (abstract->context, "Unexpected ready byte."); return DC_STATUS_PROTOCOL;
I assume in service mode, S_READY is returned instead of READY? If that's correct, then I think it's better to accept only the correct one. Something like this:
unsigned char expected = (device->state == SERVICE ? S_READY : READY); if (ready[0] != expected) { ... }
Jef
On 15 December, 2014 - Jef Driesen wrote:
On 21-11-14 21:28, Anton Lundin wrote:
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index 7fa38f8..4d6bf83 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -50,6 +50,7 @@ #define RB_LOGBOOK_SIZE 256 #define RB_LOGBOOK_COUNT 256
+#define S_READY 0x4C #define READY 0x4D #define HEADER 0x61 #define CLOCK 0x62 @@ -71,6 +72,7 @@ typedef struct hw_ostc3_device_t { NONE, OPEN, DOWNLOAD,
SERVICE,} hw_ostc3_device_t;
@@ -196,7 +198,7 @@ hw_ostc3_transfer (hw_ostc3_device_t *device, }
// Verify the ready byte.
if (ready[0] != READY) {
if (!(ready[0] == READY || ready[0] == S_READY)) { ERROR (abstract->context, "Unexpected ready byte."); return DC_STATUS_PROTOCOL;I assume in service mode, S_READY is returned instead of READY? If that's correct, then I think it's better to accept only the correct one. Something like this:
unsigned char expected = (device->state == SERVICE ? S_READY : READY); if (ready[0] != expected) { ... }
You guessed correctly. This was a artifact from the previous code where the device state was unknown to the hw_ostc3_transfer() function.
Fixed.
//Anton
Signed-off-by: Anton Lundin glance@acc.umu.se --- src/array.c | 19 +++++++++++++++++++ src/array.h | 6 ++++++ 2 files changed, 25 insertions(+)
diff --git a/src/array.c b/src/array.c index 243ec34..ee68b93 100644 --- a/src/array.c +++ b/src/array.c @@ -162,6 +162,16 @@ array_uint32_le (const unsigned char data[])
unsigned int +uint32_le_array (const unsigned int input, unsigned char data[]) +{ + data[0] = input & 0xFF; + data[1] = input >> 8 & 0xFF; + data[2] = input >> 16 & 0xFF; + data[3] = input >> 24 & 0xFF; +} + + +unsigned int array_uint24_be (const unsigned char data[]) { return (data[0] << 16) + (data[1] << 8) + data[2]; @@ -169,6 +179,15 @@ array_uint24_be (const unsigned char data[])
unsigned int +uint24_be_array (const unsigned int input, unsigned char data[]) +{ + data[0] = input >> 16 & 0xFF; + data[1] = input >> 8 & 0xFF; + data[2] = input & 0xFF; +} + + +unsigned int array_uint24_le (const unsigned char data[]) { return data[0] + (data[1] << 8) + (data[2] << 16); diff --git a/src/array.h b/src/array.h index b08ed2e..697b87e 100644 --- a/src/array.h +++ b/src/array.h @@ -56,9 +56,15 @@ unsigned int array_uint32_le (const unsigned char data[]);
unsigned int +uint32_le_array (const unsigned int input, unsigned char data[]); + +unsigned int array_uint24_be (const unsigned char data[]);
unsigned int +uint24_be_array (const unsigned int input, unsigned char data[]); + +unsigned int array_uint24_le (const unsigned char data[]);
unsigned short
On 21-11-14 21:28, Anton Lundin wrote:
unsigned int +uint32_le_array (const unsigned int input, unsigned char data[])
The array part in the function name is supposed to be the file prefix. So in that regard your functions are named a bit strange. Can you rename them to array_uintXX_{le,be}_set? And while doing that, swap the order of the two parameters, and make the function return value void.
- data[0] = input & 0xFF;
- data[1] = input >> 8 & 0xFF;
- data[2] = input >> 16 & 0xFF;
- data[3] = input >> 24 & 0xFF;
I also like to see parentheses around the shift. Not strictly necessary, but it leaves no doubt for the reader.
Jef
On 15 December, 2014 - Jef Driesen wrote:
On 21-11-14 21:28, Anton Lundin wrote:
unsigned int +uint32_le_array (const unsigned int input, unsigned char data[])
The array part in the function name is supposed to be the file prefix. So in that regard your functions are named a bit strange. Can you rename them to array_uintXX_{le,be}_set? And while doing that, swap the order of the two parameters, and make the function return value void.
As you wish. Fixed
- data[0] = input & 0xFF;
- data[1] = input >> 8 & 0xFF;
- data[2] = input >> 16 & 0xFF;
- data[3] = input >> 24 & 0xFF;
I also like to see parentheses around the shift. Not strictly necessary, but it leaves no doubt for the reader.
Fixed.
//Anton
This is the fist step in the firmware upgrade process.
This code is inspired by JeanDo ostc-companion.
Signed-off-by: Anton Lundin glance@acc.umu.se --- src/hw_ostc3.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index 4d6bf83..f647179 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -46,10 +46,13 @@ #define SZ_MEMORY 0x200000 #define SZ_CONFIG 4 #define SZ_FIRMWARE 0x01E000 // 120KB +#define SZ_FIRMWARE_BLOCK 0x1000 // 4KB +#define FIRMWARE_AREA 0x3E0000
#define RB_LOGBOOK_SIZE 256 #define RB_LOGBOOK_COUNT 256
+#define ERASE_RANGE 0x42 #define S_READY 0x4C #define READY 0x4D #define HEADER 0x61 @@ -871,3 +874,18 @@ hw_ostc3_device_service_mode (dc_device_t *abstract)
return DC_STATUS_SUCCESS; } + +static dc_status_t +hw_ostc3_device_erase_range (hw_ostc3_device_t *device, unsigned int addr, unsigned int size) +{ + dc_status_t rc = DC_STATUS_SUCCESS; + // Convert size to number of pages, rounded up. + unsigned char blocks = ((size + SZ_FIRMWARE_BLOCK - 1) / SZ_FIRMWARE_BLOCK); + + // Erase just the needed pages. + unsigned char buffer[4]; + uint24_be_array(addr, buffer); + buffer[3] = blocks; + + return hw_ostc3_transfer (device, NULL, ERASE_RANGE, buffer, sizeof(buffer), NULL, 0); +}
This is necessary to verify that the memory written got transfered correctly.
This code is inspired by JeanDo ostc-companion.
Signed-off-by: Anton Lundin glance@acc.umu.se --- src/hw_ostc3.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index f647179..d8ee534 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -52,6 +52,7 @@ #define RB_LOGBOOK_SIZE 256 #define RB_LOGBOOK_COUNT 256
+#define READ_BLOCK 0x20 #define ERASE_RANGE 0x42 #define S_READY 0x4C #define READY 0x4D @@ -889,3 +890,13 @@ hw_ostc3_device_erase_range (hw_ostc3_device_t *device, unsigned int addr, unsig
return hw_ostc3_transfer (device, NULL, ERASE_RANGE, buffer, sizeof(buffer), NULL, 0); } + +static dc_status_t +hw_ostc3_device_read_block (hw_ostc3_device_t *device, unsigned int addr, unsigned char block[], unsigned int block_size) +{ + unsigned char buffer[6]; + uint24_be_array(addr, buffer); + uint24_be_array(block_size, buffer + 3); + + return hw_ostc3_transfer (device, NULL, READ_BLOCK, buffer, sizeof(buffer), block, block_size); +}
This is now you transfer a new firmware to the OSTC3.
This code is inspired by JeanDo ostc-companion.
Signed-off-by: Anton Lundin glance@acc.umu.se --- src/hw_ostc3.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index d8ee534..df537be 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -53,6 +53,7 @@ #define RB_LOGBOOK_COUNT 256
#define READ_BLOCK 0x20 +#define WRITE_BLOCK 0x30 #define ERASE_RANGE 0x42 #define S_READY 0x4C #define READY 0x4D @@ -900,3 +901,58 @@ hw_ostc3_device_read_block (hw_ostc3_device_t *device, unsigned int addr, unsign
return hw_ostc3_transfer (device, NULL, READ_BLOCK, buffer, sizeof(buffer), block, block_size); } + +static dc_status_t +hw_ostc3_device_write_block (dc_device_t *abstract, unsigned int addr, unsigned char block[], unsigned int block_size) +{ + dc_status_t rc = DC_STATUS_SUCCESS; + hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_context_t *context = (abstract ? abstract->context : NULL); +#if 0 + // FIXME: This variant doens't work + // Things doesn't get written to memory. Timing issues? + // Version below works, serial_sleep (2) is probably the thing. + unsigned char buffer[4 + block_size]; + buffer[0] = WRITE_BLOCK; + uint24_be_array(addr, buffer + 1); + memcpy(buffer + 4, block, block_size); + + rc = hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer, sizeof(buffer), NULL, 0); + serial_sleep (device->port, 1100); + return rc; +#else + int n; + unsigned char response; + unsigned char buffer[4]; + buffer[0] = WRITE_BLOCK; + uint24_be_array(addr, buffer + 1); + + n = serial_write (device->port, buffer, sizeof(buffer)); + if (n != sizeof(buffer)) { + ERROR (context, "failed to send address to device"); + return EXITCODE (n); + } + + serial_flush (device->port, SERIAL_QUEUE_BOTH); + serial_sleep (device->port, 2); + + // Send the new block + n = serial_write (device->port, block, block_size); + if (n != block_size) { + ERROR (context, "failed to send block to device"); + return EXITCODE (n); + } + + serial_flush (device->port, SERIAL_QUEUE_BOTH); + + // A sleep lenght copied from ostc-companion + // Feels a bit slower than that what that does? + serial_sleep (device->port, 1100); + + n = serial_read (device->port, &response, sizeof (response)); + if (n != sizeof (response) || response != S_READY) + return EXITCODE (n); + + return DC_STATUS_SUCCESS; +#endif +}
On 2014-11-21 21:28, Anton Lundin wrote:
+static dc_status_t +hw_ostc3_device_write_block (dc_device_t *abstract, unsigned int addr, unsigned char block[], unsigned int block_size) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
+#if 0
- // FIXME: This variant doens't work
- // Things doesn't get written to memory. Timing issues?
- // Version below works, serial_sleep (2) is probably the thing.
- unsigned char buffer[4 + block_size];
- buffer[0] = WRITE_BLOCK;
- uint24_be_array(addr, buffer + 1);
- memcpy(buffer + 4, block, block_size);
- rc = hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer,
sizeof(buffer), NULL, 0);
- serial_sleep (device->port, 1100);
- return rc;
+#else
- int n;
- unsigned char response;
- unsigned char buffer[4];
- buffer[0] = WRITE_BLOCK;
- uint24_be_array(addr, buffer + 1);
- n = serial_write (device->port, buffer, sizeof(buffer));
- if (n != sizeof(buffer)) {
ERROR (context, "failed to send address to device");return EXITCODE (n);- }
- serial_flush (device->port, SERIAL_QUEUE_BOTH);
- serial_sleep (device->port, 2);
- // Send the new block
- n = serial_write (device->port, block, block_size);
- if (n != block_size) {
ERROR (context, "failed to send block to device");return EXITCODE (n);- }
- serial_flush (device->port, SERIAL_QUEUE_BOTH);
Do we really need those two flushes? They might cause serious problems. When writing data to a serial port, the data ends up in the output buffer of the driver/hardware, and is transmitted async. Thus when the write call returns, the data is not always guaranteed to be transmitted yet. That's why libdivecomputer also calls tcdrain internally, but even that is not always reliable.
Normally this isn't a real problem, because if the data isn't transmitted yet, the next read, to receive the response, will just take a bit longer. But if you flush the input/output buffers, then you might throw away the data you just send. And if the response arrives fast, you might throw that away as well.
The serial_flush() call is typically only necessary after opening the serial port, in order to discard any garbage data that may have been generated while connecting the serial port, or data that somehow was somehow still sitting in the internal buffers. The second use case is when receiving invalid packets. Then you flush the buffers in order in order to discard all received data and start again from a clean state. But neither of that applies here.
I just checked the ostc companion source, and their implementation of the flush function does something completely different than the libdivecomputer one. They call tcdrain internally. The libdivecomputer flush function corresponds to the purge function in the ostc companion. So I'm pretty sure that confirms those two serial_flush() calls should be removed.
I'm still surprised by the need for that 2ms delay. But okay, if it's necessary then so be it. Note that on Windows the delay will be a lot longer than those 2ms. A Windows Sleep() call will typically take at least 10-15ms.
- // A sleep lenght copied from ostc-companion
- // Feels a bit slower than that what that does?
- serial_sleep (device->port, 1100);
Why do we need this one? I might be wrong, but I think it does something different than what you think. Based on the comments in the companion code, it waits the time it takes to write the block to flash. I'm not exactly sure how it's calculated.
In theory, the 3000ms timeout we're using for reading data should be able to handle this delay already. Unless the S_READY is send much later. You should be able to see this in the timings when you enable the libdivecomputer debug log.
Another thing I noticed is that your sequence is quite different from the companion one. When mapping the companion calls to the equivalent libdivecomputer calls, it does this:
sleep(100) flush(SERIAL_QUEUE_BOTH)
write(buffer + 0, 1) read(echo, 1) write(buffer + 1,3) sleep(2) write(block, block_size) sleep(1100) read(S_READY,1)
The first sleep and flush are most likely unnecessary. Such a cleanup is only necessary when something went wrong. Since we have no code to recover from errors, that's not necessary.
With the exception of those sleep calls, that's identical to the hw_ostc3_transfer() function. Notice how your code is missing the read of the echo. I'm pretty sure that got lost due to the flushing. So this probably works correctly only by accident.
The reason why hw_ostc3_transfer doesn't work for you, might be because you include the WRITE_BLOCK byte in the input buffer. That causes it to send twice. The ostc probably doesn't like that. Can you have a look at this? I have the feeling that we can use hw_ostc3_transfer here after all. Worst case we may need to add those sleep calls there, wrapped in a "if (cmd == WRITE_BLOCK)".
- n = serial_read (device->port, &response, sizeof (response));
- if (n != sizeof (response) || response != S_READY)
return EXITCODE (n);
You should split those checks in two parts. Right now you'll return DC_STATUS_TIMEOUT when response!=S_READY, and that's misleading. If we can re-use hw_ostc3_transfer, then this is no longer an issue.
Jef
On 16 December, 2014 - Jef Driesen wrote:
On 2014-11-21 21:28, Anton Lundin wrote:
+static dc_status_t +hw_ostc3_device_write_block (dc_device_t *abstract, unsigned int addr, unsigned char block[], unsigned int block_size) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
+#if 0
- // FIXME: This variant doens't work
- // Things doesn't get written to memory. Timing issues?
- // Version below works, serial_sleep (2) is probably the thing.
- unsigned char buffer[4 + block_size];
- buffer[0] = WRITE_BLOCK;
- uint24_be_array(addr, buffer + 1);
- memcpy(buffer + 4, block, block_size);
- rc = hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer,
sizeof(buffer), NULL, 0);
- serial_sleep (device->port, 1100);
- return rc;
+#else
...
With the exception of those sleep calls, that's identical to the hw_ostc3_transfer() function. Notice how your code is missing the read of the echo. I'm pretty sure that got lost due to the flushing. So this probably works correctly only by accident.
The reason why hw_ostc3_transfer doesn't work for you, might be because you include the WRITE_BLOCK byte in the input buffer. That causes it to send twice. The ostc probably doesn't like that. Can you have a look at this? I have the feeling that we can use hw_ostc3_transfer here after all. Worst case we may need to add those sleep calls there, wrapped in a "if (cmd == WRITE_BLOCK)".
- n = serial_read (device->port, &response, sizeof (response));
- if (n != sizeof (response) || response != S_READY)
return EXITCODE (n);You should split those checks in two parts. Right now you'll return DC_STATUS_TIMEOUT when response!=S_READY, and that's misleading. If we can re-use hw_ostc3_transfer, then this is no longer an issue.
After i started to rewrite the code for like the .. 7th time i spotted the bug in the hw_ostc3_transfer-based variant, and indeed it was the extra WRITE_BLOCK i never spotted.
Now the hw_ostc3_device_write_block-helper looks like this:
static dc_status_t hw_ostc3_device_write_block (hw_ostc3_device_t *device, unsigned int addr, unsigned char block[], unsigned int block_size) { unsigned char buffer[3 + block_size]; array_uint24_be_set(buffer, addr); memcpy(buffer + 3, block, block_size);
return hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer, sizeof(buffer), NULL, 0); }
Just like hw_ostc3_device_read_block, and works just fine.
//Anton
On 16-12-14 20:06, Anton Lundin wrote:
On 16 December, 2014 - Jef Driesen wrote:
On 2014-11-21 21:28, Anton Lundin wrote:
+static dc_status_t +hw_ostc3_device_write_block (dc_device_t *abstract, unsigned int addr, unsigned char block[], unsigned int block_size) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
+#if 0
- // FIXME: This variant doens't work
- // Things doesn't get written to memory. Timing issues?
- // Version below works, serial_sleep (2) is probably the thing.
- unsigned char buffer[4 + block_size];
- buffer[0] = WRITE_BLOCK;
- uint24_be_array(addr, buffer + 1);
- memcpy(buffer + 4, block, block_size);
- rc = hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer,
sizeof(buffer), NULL, 0);
- serial_sleep (device->port, 1100);
- return rc;
+#else
...
With the exception of those sleep calls, that's identical to the hw_ostc3_transfer() function. Notice how your code is missing the read of the echo. I'm pretty sure that got lost due to the flushing. So this probably works correctly only by accident.
The reason why hw_ostc3_transfer doesn't work for you, might be because you include the WRITE_BLOCK byte in the input buffer. That causes it to send twice. The ostc probably doesn't like that. Can you have a look at this? I have the feeling that we can use hw_ostc3_transfer here after all. Worst case we may need to add those sleep calls there, wrapped in a "if (cmd == WRITE_BLOCK)".
- n = serial_read (device->port, &response, sizeof (response));
- if (n != sizeof (response) || response != S_READY)
return EXITCODE (n);You should split those checks in two parts. Right now you'll return DC_STATUS_TIMEOUT when response!=S_READY, and that's misleading. If we can re-use hw_ostc3_transfer, then this is no longer an issue.
After i started to rewrite the code for like the .. 7th time i spotted the bug in the hw_ostc3_transfer-based variant, and indeed it was the extra WRITE_BLOCK i never spotted.
Now the hw_ostc3_device_write_block-helper looks like this:
static dc_status_t hw_ostc3_device_write_block (hw_ostc3_device_t *device, unsigned int addr, unsigned char block[], unsigned int block_size) { unsigned char buffer[3 + block_size]; array_uint24_be_set(buffer, addr); memcpy(buffer + 3, block, block_size);
return hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer, sizeof(buffer), NULL, 0); }
Just like hw_ostc3_device_read_block, and works just fine.
Excellent news! Just one comment. Use a fixed size array here. Variable Length Arrays (VLA) are not supported in C++, so this breaks msvc compatibility :-(
There are also a few other minor issues for msvc. See the attached patch.
(For the snprintf function, I should make some helper function, because the semantics of the msvc _snprintf is quite different. For example it does not always null terminated the string, which is really bad. What the hell where they thinking?)
Jef
On 17 December, 2014 - Jef Driesen wrote:
On 16-12-14 20:06, Anton Lundin wrote:
On 16 December, 2014 - Jef Driesen wrote:
On 2014-11-21 21:28, Anton Lundin wrote:
+static dc_status_t +hw_ostc3_device_write_block (dc_device_t *abstract, unsigned int addr, unsigned char block[], unsigned int block_size) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
+#if 0
- // FIXME: This variant doens't work
- // Things doesn't get written to memory. Timing issues?
- // Version below works, serial_sleep (2) is probably the thing.
- unsigned char buffer[4 + block_size];
- buffer[0] = WRITE_BLOCK;
- uint24_be_array(addr, buffer + 1);
- memcpy(buffer + 4, block, block_size);
- rc = hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer,
sizeof(buffer), NULL, 0);
- serial_sleep (device->port, 1100);
- return rc;
+#else
...
With the exception of those sleep calls, that's identical to the hw_ostc3_transfer() function. Notice how your code is missing the read of the echo. I'm pretty sure that got lost due to the flushing. So this probably works correctly only by accident.
The reason why hw_ostc3_transfer doesn't work for you, might be because you include the WRITE_BLOCK byte in the input buffer. That causes it to send twice. The ostc probably doesn't like that. Can you have a look at this? I have the feeling that we can use hw_ostc3_transfer here after all. Worst case we may need to add those sleep calls there, wrapped in a "if (cmd == WRITE_BLOCK)".
- n = serial_read (device->port, &response, sizeof (response));
- if (n != sizeof (response) || response != S_READY)
return EXITCODE (n);You should split those checks in two parts. Right now you'll return DC_STATUS_TIMEOUT when response!=S_READY, and that's misleading. If we can re-use hw_ostc3_transfer, then this is no longer an issue.
After i started to rewrite the code for like the .. 7th time i spotted the bug in the hw_ostc3_transfer-based variant, and indeed it was the extra WRITE_BLOCK i never spotted.
Now the hw_ostc3_device_write_block-helper looks like this:
static dc_status_t hw_ostc3_device_write_block (hw_ostc3_device_t *device, unsigned int addr, unsigned char block[], unsigned int block_size) { unsigned char buffer[3 + block_size]; array_uint24_be_set(buffer, addr); memcpy(buffer + 3, block, block_size);
return hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer, sizeof(buffer), NULL, 0); }
Just like hw_ostc3_device_read_block, and works just fine.
Excellent news! Just one comment. Use a fixed size array here. Variable Length Arrays (VLA) are not supported in C++, so this breaks msvc compatibility :-(
I constrained to only being capable of writing SZ_FIRMWARE_BLOCK sized blocks.
There are also a few other minor issues for msvc. See the attached patch.
Fixed.
//Anton
This function triggers a reboot into the bootloader which flashes the new firmware to Prom.
This code is inspired by JeanDo ostc-companion.
Signed-off-by: Anton Lundin glance@acc.umu.se --- src/hw_ostc3.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index df537be..5ca26f2 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -57,6 +57,7 @@ #define ERASE_RANGE 0x42 #define S_READY 0x4C #define READY 0x4D +#define FLASH_FIRM 0x50 #define HEADER 0x61 #define CLOCK 0x62 #define CUSTOMTEXT 0x63 @@ -956,3 +957,37 @@ hw_ostc3_device_write_block (dc_device_t *abstract, unsigned int addr, unsigned return DC_STATUS_SUCCESS; #endif } + +static dc_status_t +hw_ostc3_device_upgrade_firmware (dc_device_t *abstract, unsigned int checksum) +{ + dc_status_t rc = DC_STATUS_SUCCESS; + hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_context_t *context = (abstract ? abstract->context : NULL); + unsigned char buffer[5]; + uint32_le_array(checksum, buffer); + + // Compute a one byte checksum, so the device can validate the firmware image. + buffer[4] = 0x55; + buffer[4] ^= buffer[0]; + buffer[4] = (buffer[4]<<1 | buffer[4]>>7); + buffer[4] ^= buffer[1]; + buffer[4] = (buffer[4]<<1 | buffer[4]>>7); + buffer[4] ^= buffer[2]; + buffer[4] = (buffer[4]<<1 | buffer[4]>>7); + buffer[4] ^= buffer[3]; + buffer[4] = (buffer[4]<<1 | buffer[4]>>7); + + rc = hw_ostc3_transfer(device, NULL, FLASH_FIRM, buffer, sizeof(buffer), NULL, 0); + if (rc != DC_STATUS_SUCCESS) { + ERROR (context, "Failed to send flash firmware command"); + return rc; + } + + // Now the device resets, and if everything is well, it reprograms. + serial_sleep (device->port, 500); + + // FIXME: How should we force the application to close the device here? + + return DC_STATUS_SUCCESS; +}
On 2014-11-21 21:28, Anton Lundin wrote:
+static dc_status_t +hw_ostc3_device_upgrade_firmware (dc_device_t *abstract, unsigned int checksum) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
- unsigned char buffer[5];
- uint32_le_array(checksum, buffer);
- // Compute a one byte checksum, so the device can validate the
firmware image.
- buffer[4] = 0x55;
- buffer[4] ^= buffer[0];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[1];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[2];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[3];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
Can you change this into a loop? It's already more than cryptic enough. Any idea whether this is some known checksum?
- // Now the device resets, and if everything is well, it reprograms.
- serial_sleep (device->port, 500);
- // FIXME: How should we force the application to close the device
here?
Why do we need to wait here? If the device is rebooting, then the firmware update is successful, right? Since we won't get any confirmation from the ostc, what's the point of waiting here? It only blocks the caller from calling close. Or am I missing something else?
What happens at this point? Does the usb-serial device node (e.g. /dev/ttyUSBx) disappear? Setting the state to REBOOTING (as you changed the FIXME in one of the other patches) prevents to do anything, except for calling close. That's probably the right thing to do, but I just wonder what's going on under the hood.
This might be different for the ostc3 (usb-serial) and sport (bluetooth), because I suspect the sport will terminate the bluetooth during the reboot, while for the ostc3 the usb-serial chip is still there.
Jef
On 16 December, 2014 - Jef Driesen wrote:
On 2014-11-21 21:28, Anton Lundin wrote:
+static dc_status_t +hw_ostc3_device_upgrade_firmware (dc_device_t *abstract, unsigned int checksum) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
- unsigned char buffer[5];
- uint32_le_array(checksum, buffer);
- // Compute a one byte checksum, so the device can validate the firmware
image.
- buffer[4] = 0x55;
- buffer[4] ^= buffer[0];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[1];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[2];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[3];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
Can you change this into a loop? It's already more than cryptic enough. Any idea whether this is some known checksum?
I've looked around and there are no hints in ostc-companion or the ostc3 firmware. I've asked around a bit if someone have seen it somewhere else, but as far as i know, its something home grown.
I've compacted it with a loop as you requested.
- // Now the device resets, and if everything is well, it reprograms.
- serial_sleep (device->port, 500);
- // FIXME: How should we force the application to close the device here?
Why do we need to wait here? If the device is rebooting, then the firmware update is successful, right? Since we won't get any confirmation from the ostc, what's the point of waiting here? It only blocks the caller from calling close. Or am I missing something else?
That sleep was probably just re-implemented from ostc-companion. Works just fine without it. Removed it.
What happens at this point? Does the usb-serial device node (e.g. /dev/ttyUSBx) disappear? Setting the state to REBOOTING (as you changed the FIXME in one of the other patches) prevents to do anything, except for calling close. That's probably the right thing to do, but I just wonder what's going on under the hood.
For the ostc3, the host computer sees the ftdi-chip all the time.
The FIXME here was just left because I've managed to squash the commit that removed it into the next one in the series, and thats why its removed there.
This might be different for the ostc3 (usb-serial) and sport (bluetooth), because I suspect the sport will terminate the bluetooth during the reboot, while for the ostc3 the usb-serial chip is still there.
Yepp, it terminates the connection.
So, what do you suggest, that we should change the REBOOTING state to CLOSED and close the device in hw_ostc3_device_upgrade_firmware?
I think that would be a really confusing flow.
//Anton
On 16-12-14 21:39, Anton Lundin wrote:
On 16 December, 2014 - Jef Driesen wrote:
On 2014-11-21 21:28, Anton Lundin wrote:
+static dc_status_t +hw_ostc3_device_upgrade_firmware (dc_device_t *abstract, unsigned int checksum) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
- unsigned char buffer[5];
- uint32_le_array(checksum, buffer);
- // Compute a one byte checksum, so the device can validate the firmware
image.
- buffer[4] = 0x55;
- buffer[4] ^= buffer[0];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[1];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[2];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
- buffer[4] ^= buffer[3];
- buffer[4] = (buffer[4]<<1 | buffer[4]>>7);
Can you change this into a loop? It's already more than cryptic enough. Any idea whether this is some known checksum?
I've looked around and there are no hints in ostc-companion or the ostc3 firmware. I've asked around a bit if someone have seen it somewhere else, but as far as i know, its something home grown.
I've compacted it with a loop as you requested.
I couldn't find anything either. It's a bit of a weird checksum. First of all it's a checksum of another checksum. Those circular shifts are a bit unusual too. But if that's what we need, then that's what we implement :-)
- // Now the device resets, and if everything is well, it reprograms.
- serial_sleep (device->port, 500);
- // FIXME: How should we force the application to close the device here?
Why do we need to wait here? If the device is rebooting, then the firmware update is successful, right? Since we won't get any confirmation from the ostc, what's the point of waiting here? It only blocks the caller from calling close. Or am I missing something else?
That sleep was probably just re-implemented from ostc-companion. Works just fine without it. Removed it.
Cool. No need to slow things down for no good reason.
What happens at this point? Does the usb-serial device node (e.g. /dev/ttyUSBx) disappear? Setting the state to REBOOTING (as you changed the FIXME in one of the other patches) prevents to do anything, except for calling close. That's probably the right thing to do, but I just wonder what's going on under the hood.
For the ostc3, the host computer sees the ftdi-chip all the time.
The FIXME here was just left because I've managed to squash the commit that removed it into the next one in the series, and thats why its removed there.
This might be different for the ostc3 (usb-serial) and sport (bluetooth), because I suspect the sport will terminate the bluetooth during the reboot, while for the ostc3 the usb-serial chip is still there.
Yepp, it terminates the connection.
Makes sense indeed.
So, what do you suggest, that we should change the REBOOTING state to CLOSED and close the device in hw_ostc3_device_upgrade_firmware?
I think that would be a really confusing flow.
No, that's not what I meant. The way you implemented it with the special REBOOTING state (or CLOSED or whatever you want to call it) is fine. Closing the actual serial connection should be done only by the close function. Otherwise we would indeed end up with a very confusing flow. The serial connection remains open for as long as the device handle is open. After a firmware update you may not be able to re-use it any longer, but that's another problem. We can check the state for that.
[For background information: For the new api design, I want to move open/close of the serial connection to the application side. The rationale behind that is to support alternative implementations (e.g. custom user-space usb-serial drivers for Android), and device enumeration. In that case the libdivecomputer device backends should certainly never close the underlying connection, because that'll become the responsibility of the application.]
Jef
This connects the bits and implements firmware upgrade for the OSTC3.
This code is inspired by JeanDo ostc-companion.
Signed-off-by: Anton Lundin glance@acc.umu.se --- include/libdivecomputer/hw_ostc3.h | 3 + src/hw_ostc3.c | 119 ++++++++++++++++++++++++++++++++++++- src/libdivecomputer.symbols | 1 + 3 files changed, 121 insertions(+), 2 deletions(-)
diff --git a/include/libdivecomputer/hw_ostc3.h b/include/libdivecomputer/hw_ostc3.h index 267b7e3..bc56a9d 100644 --- a/include/libdivecomputer/hw_ostc3.h +++ b/include/libdivecomputer/hw_ostc3.h @@ -58,6 +58,9 @@ hw_ostc3_device_config_write (dc_device_t *abstract, unsigned int config, const dc_status_t hw_ostc3_device_config_reset (dc_device_t *abstract);
+dc_status_t +hw_ostc3_device_fwupdate (dc_device_t *abstract, const char *filename); + #ifdef __cplusplus } #endif /* __cplusplus */ diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index 5ca26f2..64a861b 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -79,6 +79,7 @@ typedef struct hw_ostc3_device_t { OPEN, DOWNLOAD, SERVICE, + REBOOTING, } state; } hw_ostc3_device_t;
@@ -319,7 +320,7 @@ hw_ostc3_device_close (dc_device_t *abstract) return DC_STATUS_INVALIDARGS;
// Send the exit command - if (device->state != OPEN) { + if (device->state != REBOOTING && device->state != OPEN) { status = hw_ostc3_transfer (device, NULL, EXIT, NULL, 0, NULL, 0); if (status != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to send the command."); @@ -987,7 +988,121 @@ hw_ostc3_device_upgrade_firmware (dc_device_t *abstract, unsigned int checksum) // Now the device resets, and if everything is well, it reprograms. serial_sleep (device->port, 500);
- // FIXME: How should we force the application to close the device here? + device->state = REBOOTING;
return DC_STATUS_SUCCESS; } + + +dc_status_t +hw_ostc3_device_fwupdate (dc_device_t *abstract, const char *filename) +{ + dc_status_t rc = DC_STATUS_SUCCESS; + hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract; + dc_context_t *context = (abstract ? abstract->context : NULL); + + // Enable progress notifications. + dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; + + if (!ISINSTANCE (abstract)) + return DC_STATUS_INVALIDARGS; + + if (device->state == OPEN) { + if ((rc = hw_ostc3_device_service_mode(abstract)) != DC_STATUS_SUCCESS) + return rc; + } else if (device->state != SERVICE) { + return DC_STATUS_INVALIDARGS; + } + + // load, erase, upload FZ, verify FZ, reprogram + progress.maximum = 3 + SZ_FIRMWARE * 2 / SZ_FIRMWARE_BLOCK; + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + + hw_ostc3_device_display(abstract, " Loading FW..."); + + // Allocate memory for the firmware data. + hw_ostc3_firmware_t *firmware = (hw_ostc3_firmware_t *) malloc (sizeof (hw_ostc3_firmware_t)); + if (firmware == NULL) { + ERROR (context, "Failed to allocate memory."); + return DC_STATUS_NOMEMORY; + } + + // Read the hex file. + rc = hw_ostc3_firmware_readfile (firmware, context, filename); + if (rc != DC_STATUS_SUCCESS) { + free (firmware); + return rc; + } + + // Firmware loaded + progress.current++; + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + + hw_ostc3_device_display(abstract, " Erasing FW..."); + rc = hw_ostc3_device_erase_range(device, FIRMWARE_AREA, SZ_FIRMWARE); + if (rc != DC_STATUS_SUCCESS) { + free (firmware); + ERROR (context, "Failed to erase old firmware"); + return rc; + } + + // Memory erased + progress.current++; + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + + hw_ostc3_device_display(abstract, " Uploading..."); + + for(int len = 0; len < SZ_FIRMWARE; len += SZ_FIRMWARE_BLOCK) + { + char status[16]; // Status message on the display + snprintf (status, 16, " Uploading %2d%%", (100 * len) / SZ_FIRMWARE); + hw_ostc3_device_display (abstract, status); + + rc = hw_ostc3_device_write_block (abstract, FIRMWARE_AREA + len, firmware->data + len, SZ_FIRMWARE_BLOCK); + if (rc != DC_STATUS_SUCCESS) { + free(firmware); + ERROR (context, "Failed to write block to device"); + return rc; + } + // One block uploaded + progress.current++; + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + } + + hw_ostc3_device_display (abstract, " Verifying..."); + + for(int len = 0; len < SZ_FIRMWARE; len += SZ_FIRMWARE_BLOCK) + { + unsigned char block[SZ_FIRMWARE_BLOCK]; + char status[16]; // Status message on the display + snprintf (status, 16, " Verifying %2d%%", (100 * len) / SZ_FIRMWARE); + hw_ostc3_device_display (abstract, status); + + rc = hw_ostc3_device_read_block (device, FIRMWARE_AREA + len, block, sizeof(block)); + if (rc != DC_STATUS_SUCCESS || memcmp (firmware->data + len, block, sizeof (block)) != 0) { + hw_ostc3_device_display (abstract, " Verify FAILED"); + free (firmware); + ERROR (context, "Failed verify."); + return DC_STATUS_IO; + } + // One block verified + progress.current++; + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + } + + hw_ostc3_device_display(abstract, " Programming..."); + + rc = hw_ostc3_device_upgrade_firmware(abstract, firmware->checksum); + if (rc != DC_STATUS_SUCCESS) { + free (firmware); + ERROR (context, "Failed to start programing"); + return rc; + } + + // Programing done! + progress.current++; + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + + // Finished! + return DC_STATUS_SUCCESS; +} diff --git a/src/libdivecomputer.symbols b/src/libdivecomputer.symbols index 14a96a8..4984942 100644 --- a/src/libdivecomputer.symbols +++ b/src/libdivecomputer.symbols @@ -157,6 +157,7 @@ hw_ostc3_device_customtext hw_ostc3_device_config_read hw_ostc3_device_config_write hw_ostc3_device_config_reset +hw_ostc3_device_fwupdate zeagle_n2ition3_device_open atomics_cobalt_device_open atomics_cobalt_device_version
On 21 November, 2014 - Anton Lundin wrote:
diff --git a/src/hw_ostc3.c b/src/hw_ostc3.c index 5ca26f2..64a861b 100644 --- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -79,6 +79,7 @@ typedef struct hw_ostc3_device_t { OPEN, DOWNLOAD, SERVICE,
REBOOTING,} hw_ostc3_device_t;
@@ -319,7 +320,7 @@ hw_ostc3_device_close (dc_device_t *abstract) return DC_STATUS_INVALIDARGS;
// Send the exit command
- if (device->state != OPEN) {
- if (device->state != REBOOTING && device->state != OPEN) { status = hw_ostc3_transfer (device, NULL, EXIT, NULL, 0, NULL, 0); if (status != DC_STATUS_SUCCESS) { ERROR (abstract->context, "Failed to send the command.");
@@ -987,7 +988,121 @@ hw_ostc3_device_upgrade_firmware (dc_device_t *abstract, unsigned int checksum) // Now the device resets, and if everything is well, it reprograms. serial_sleep (device->port, 500);
- // FIXME: How should we force the application to close the device here?
device->state = REBOOTING;
return DC_STATUS_SUCCESS;
}
I found this slip-up and fixed it. This should have bin a part of "Add a function upgrade the firmware in the OSTC3"
//Anton
On 2014-11-21 21:28, Anton Lundin wrote:
+dc_status_t +hw_ostc3_device_fwupdate (dc_device_t *abstract, const char *filename) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
- // Enable progress notifications.
- dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER;
- if (!ISINSTANCE (abstract))
return DC_STATUS_INVALIDARGS;- if (device->state == OPEN) {
if ((rc = hw_ostc3_device_service_mode(abstract)) !=DC_STATUS_SUCCESS)
return rc;- } else if (device->state != SERVICE) {
return DC_STATUS_INVALIDARGS;- }
- // load, erase, upload FZ, verify FZ, reprogram
- progress.maximum = 3 + SZ_FIRMWARE * 2 / SZ_FIRMWARE_BLOCK;
- device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
- hw_ostc3_device_display(abstract, " Loading FW...");
- // Allocate memory for the firmware data.
- hw_ostc3_firmware_t *firmware = (hw_ostc3_firmware_t *) malloc
(sizeof (hw_ostc3_firmware_t));
- if (firmware == NULL) {
ERROR (context, "Failed to allocate memory.");return DC_STATUS_NOMEMORY;- }
- // Read the hex file.
- rc = hw_ostc3_firmware_readfile (firmware, context, filename);
- if (rc != DC_STATUS_SUCCESS) {
free (firmware);return rc;- }
- // Firmware loaded
- progress.current++;
- device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
I think it's better to load the firmware file before trying to boot into service mode. If there is anything wrong with the firmware file, then there is no point to boot into service mode. That might save user some trouble.
Reading the firmware file should be instant compared to the time it takes to flash it. So the fact that we won't reporting any progress for this stage won't be an issue.
BTW, how do you exit service mode? Here, I'm thinking about the case where something goes wrong and we have to abort the firmware upgrade. If I understand correctly, the original firmware will be preserved, so the device can't be bricked. But I assume it's more re-assuring to the user if the device boots again, rather than staying in service mode. Note that I haven't tested the firmware update yet, so if this is something obvious just ignore the question :-)
- hw_ostc3_device_display(abstract, " Erasing FW...");
I'm not really sure what's the best way to deal with these notifications. When doing this from within libdivecomputer, then it won't be possible to localize these messages. That might be confusing because the ostc firmware is available in several languages. Doing this on the application is not impossible, but there you only have access to the progress events, so you could show the overall percentage but not the stage (e.g. erasing, uploading, verifying and flashing).
Suggestions are welcome.
rc = hw_ostc3_device_read_block (device, FIRMWARE_AREA + len,block, sizeof(block));
if (rc != DC_STATUS_SUCCESS || memcmp (firmware->data + len, block,sizeof (block)) != 0) {
hw_ostc3_device_display (abstract, " Verify FAILED");free (firmware);ERROR (context, "Failed verify.");return DC_STATUS_IO;}
If hw_ostc3_device_read_block() fails, you should return rc, like you already do elsewhere. Not DC_STATUS_IO. And when the memcmp() fails, the most appropriate error code is DC_STATUS_PROTOCOL.
Jef
On 16 December, 2014 - Jef Driesen wrote:
On 2014-11-21 21:28, Anton Lundin wrote:
+dc_status_t +hw_ostc3_device_fwupdate (dc_device_t *abstract, const char *filename) +{
- dc_status_t rc = DC_STATUS_SUCCESS;
- hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
- // Enable progress notifications.
- dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER;
- if (!ISINSTANCE (abstract))
return DC_STATUS_INVALIDARGS;- if (device->state == OPEN) {
if ((rc = hw_ostc3_device_service_mode(abstract)) != DC_STATUS_SUCCESS)return rc;- } else if (device->state != SERVICE) {
return DC_STATUS_INVALIDARGS;- }
- // load, erase, upload FZ, verify FZ, reprogram
- progress.maximum = 3 + SZ_FIRMWARE * 2 / SZ_FIRMWARE_BLOCK;
- device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
- hw_ostc3_device_display(abstract, " Loading FW...");
- // Allocate memory for the firmware data.
- hw_ostc3_firmware_t *firmware = (hw_ostc3_firmware_t *) malloc
(sizeof (hw_ostc3_firmware_t));
- if (firmware == NULL) {
ERROR (context, "Failed to allocate memory.");return DC_STATUS_NOMEMORY;- }
- // Read the hex file.
- rc = hw_ostc3_firmware_readfile (firmware, context, filename);
- if (rc != DC_STATUS_SUCCESS) {
free (firmware);return rc;- }
- // Firmware loaded
- progress.current++;
- device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
I think it's better to load the firmware file before trying to boot into service mode. If there is anything wrong with the firmware file, then there is no point to boot into service mode. That might save user some trouble.
Thats true. If a application where to open the device, and then call hw_ostc3_device_fwupdate() and the read firmware call fails due to ex. user choosing the wrong file, the application may want to keep the dc descriptor open and do something else.
Reading the firmware file should be instant compared to the time it takes to flash it. So the fact that we won't reporting any progress for this stage won't be an issue.
BTW, how do you exit service mode? Here, I'm thinking about the case where something goes wrong and we have to abort the firmware upgrade. If I understand correctly, the original firmware will be preserved, so the device can't be bricked. But I assume it's more re-assuring to the user if the device boots again, rather than staying in service mode. Note that I haven't tested the firmware update yet, so if this is something obvious just ignore the question :-)
You can either exit it by sending EXIT, as done in hw_ostc3_device_close, or when unplugging the device from the usb-port.
The firmware upgrade is done by the bootloader in the device, and only after the command to boot into the bootloader is sent, so you can abort the upgrade process at any point and not brick the device.
- hw_ostc3_device_display(abstract, " Erasing FW...");
I'm not really sure what's the best way to deal with these notifications. When doing this from within libdivecomputer, then it won't be possible to localize these messages. That might be confusing because the ostc firmware is available in several languages. Doing this on the application is not impossible, but there you only have access to the progress events, so you could show the overall percentage but not the stage (e.g. erasing, uploading, verifying and flashing).
Suggestions are welcome.
I'd say its really nice to have some feedback on the device, thats why i copied that pattern from ostc-companion and implemented it here. The strings are also a bit tricky to craft due to the length limitations so I'm just fine with the strings being in English and "hard-coded".
If there where a way to send more generic callbacks to the app, it would be great to send the current state that way too, but you don't want the app calling hw_ostc3-functions in the middle of your firmware upgrade process.
I'd say we keep em as-is until someone comes up with a better solution.
rc = hw_ostc3_device_read_block (device, FIRMWARE_AREA + len,block, sizeof(block));
if (rc != DC_STATUS_SUCCESS || memcmp (firmware->data + len, block,sizeof (block)) != 0) {
hw_ostc3_device_display (abstract, " Verify FAILED");free (firmware);ERROR (context, "Failed verify.");return DC_STATUS_IO;}If hw_ostc3_device_read_block() fails, you should return rc, like you already do elsewhere. Not DC_STATUS_IO. And when the memcmp() fails, the most appropriate error code is DC_STATUS_PROTOCOL.
Split them now.
//Anton
This teaches the example firmware upgrader about how to upgrade the OSTC3's.
Signed-off-by: Anton Lundin glance@acc.umu.se --- examples/hw_ostc_fwupdate.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/examples/hw_ostc_fwupdate.c b/examples/hw_ostc_fwupdate.c index a9e7fde..7ce1347 100644 --- a/examples/hw_ostc_fwupdate.c +++ b/examples/hw_ostc_fwupdate.c @@ -19,10 +19,10 @@ * MA 02110-1301 USA */
-#include <stdio.h> // fopen, fwrite, fclose -#include <stdlib.h> +#include <string.h>
#include <libdivecomputer/hw_ostc.h> +#include <libdivecomputer/hw_ostc3.h>
#include "utils.h" #include "common.h" @@ -44,17 +44,23 @@ event_cb (dc_device_t *device, dc_event_type_t event, const void *data, void *us }
static dc_status_t -fwupdate (const char *name, const char *hexfile) +fwupdate (const char *name, const char *hexfile, int ostc3) { dc_context_t *context = NULL; dc_device_t *device = NULL; + dc_status_t rc = DC_EVENT_PROGRESS;
dc_context_new (&context); dc_context_set_loglevel (context, DC_LOGLEVEL_ALL); dc_context_set_logfunc (context, logfunc, NULL);
- message ("hw_ostc_device_open\n"); - dc_status_t rc = hw_ostc_device_open (&device, context, name); + if (ostc3) { + message ("hw_ostc3_device_open\n"); + rc = hw_ostc3_device_open (&device, context, name); + } else { + message ("hw_ostc_device_open\n"); + rc = hw_ostc_device_open (&device, context, name); + } if (rc != DC_STATUS_SUCCESS) { WARNING ("Error opening serial port."); dc_context_free (context); @@ -70,8 +76,13 @@ fwupdate (const char *name, const char *hexfile) return rc; }
- message ("hw_ostc_device_fwupdate\n"); - rc = hw_ostc_device_fwupdate (device, hexfile); + if (ostc3) { + message ("hw_ostc3_device_fwupdate\n"); + rc = hw_ostc3_device_fwupdate (device, hexfile); + } else { + message ("hw_ostc_device_fwupdate\n"); + rc = hw_ostc_device_fwupdate (device, hexfile); + } if (rc != DC_STATUS_SUCCESS) { WARNING ("Error flashing firmware."); dc_device_close (device); @@ -103,6 +114,7 @@ int main(int argc, char *argv[]) const char* name = "/dev/ttyUSB0"; #endif const char *hexfile = NULL; + int ostc3 = 0;
if (argc > 1) { name = argv[1]; @@ -110,11 +122,18 @@ int main(int argc, char *argv[]) if (argc > 2) { hexfile = argv[2]; } + if (argc > 3) { + if (strcmp(argv[3], "-3") == 0) { + ostc3 = 1; + } else { + ostc3 = 0; + } + }
message ("DEVICE=%s\n", name); message ("HEXFILE=%s\n", hexfile);
- dc_status_t a = fwupdate (name, hexfile); + dc_status_t a = fwupdate (name, hexfile, ostc3);
message ("SUMMARY\n"); message ("-------\n");
On 21 November, 2014 - Anton Lundin wrote:
This teaches the example firmware upgrader about how to upgrade the OSTC3's.
Signed-off-by: Anton Lundin glance@acc.umu.se
This patch is sort of the ugly duck of this patch series. It works<tm> but its not that pretty.
Grab it if you would like or just skip it and write something thats less-ugly.
//Anton
On 2014-11-21 21:36, Anton Lundin wrote:
On 21 November, 2014 - Anton Lundin wrote:
This teaches the example firmware upgrader about how to upgrade the OSTC3's.
Signed-off-by: Anton Lundin glance@acc.umu.se
This patch is sort of the ugly duck of this patch series. It works<tm> but its not that pretty.
Grab it if you would like or just skip it and write something thats less-ugly.
The new dctool application will include support for firmware updates in a nicer way. Once it's there, this ostc fwupdate tool will get removed. But for the time being it's better than nothing. Just don't spend too much time polishing it :-)
Jef
On 21-11-14 21:28, Anton Lundin wrote:
This imports Tiny AES128 from https://github.com/kokke/tiny-AES128-C for use in the decoding of OSTC3 firmwares.
There are two problems with this aes implementation.
The first one is the stdint.h header, which is not available when compiling with msvc. This can be fixed easily by replacing uint8_t and uint32_t with unsigned char and unsigned int. (This is also the main reason why libdivecomputer doesn't use those C99 integer types anywhere.)
The second one are the global variables. This is not thread-safe, so this will need to be refactored to move those global variables into some aes_ctx_t structure, and pass that to the helper functions instead. The alternative is to link against some crypto library (e.g. openssl). But that might be overkill for what we need.
Jef
On 15 December, 2014 - Jef Driesen wrote:
On 21-11-14 21:28, Anton Lundin wrote:
This imports Tiny AES128 from https://github.com/kokke/tiny-AES128-C for use in the decoding of OSTC3 firmwares.
There are two problems with this aes implementation.
The first one is the stdint.h header, which is not available when compiling with msvc. This can be fixed easily by replacing uint8_t and uint32_t with unsigned char and unsigned int. (This is also the main reason why libdivecomputer doesn't use those C99 integer types anywhere.)
Wouldn't it actually be better if you either typedef'ed all the needed types yourself in a stdint-for-msvc.h ?
Or even, started using <cstdint> ?
The second one are the global variables. This is not thread-safe, so this will need to be refactored to move those global variables into some aes_ctx_t structure, and pass that to the helper functions instead. The alternative is to link against some crypto library (e.g. openssl). But that might be overkill for what we need.
Its a simpe thing to switch to openssl/aes.h and AES_encrypt()/AES_decrypt() but then Debian and co will go mental about linking gpl software to openssl...
Yea, i know its a bridge to pass, but do firmware code _really_ need to be thread safe?
//Anton
On 16 December, 2014 - Anton Lundin wrote:
On 15 December, 2014 - Jef Driesen wrote:
On 21-11-14 21:28, Anton Lundin wrote:
This imports Tiny AES128 from https://github.com/kokke/tiny-AES128-C for use in the decoding of OSTC3 firmwares.
There are two problems with this aes implementation.
The first one is the stdint.h header, which is not available when compiling with msvc. This can be fixed easily by replacing uint8_t and uint32_t with unsigned char and unsigned int. (This is also the main reason why libdivecomputer doesn't use those C99 integer types anywhere.)
Wouldn't it actually be better if you either typedef'ed all the needed types yourself in a stdint-for-msvc.h ?
Or even, started using <cstdint> ?
The second one are the global variables. This is not thread-safe, so this will need to be refactored to move those global variables into some aes_ctx_t structure, and pass that to the helper functions instead. The alternative is to link against some crypto library (e.g. openssl). But that might be overkill for what we need.
Its a simpe thing to switch to openssl/aes.h and AES_encrypt()/AES_decrypt() but then Debian and co will go mental about linking gpl software to openssl...
Yea, i know its a bridge to pass, but do firmware code _really_ need to be thread safe?
Late last night i took a stab at, and did a quite quick'n'dirty change to tiny-AES128-C to move the static state varables to a state struct and allocate that one on the stack instead.
I suggested the idea to upstream and we will se what they say, but they aren't completely against the idea:
https://github.com/kokke/tiny-AES128-C/pull/9
Or we could always carry such code as a local patch.
//Anton
On 17-12-14 10:10, Anton Lundin wrote:
On 16 December, 2014 - Anton Lundin wrote:
On 15 December, 2014 - Jef Driesen wrote:
On 21-11-14 21:28, Anton Lundin wrote:
This imports Tiny AES128 from https://github.com/kokke/tiny-AES128-C for use in the decoding of OSTC3 firmwares.
There are two problems with this aes implementation.
The first one is the stdint.h header, which is not available when compiling with msvc. This can be fixed easily by replacing uint8_t and uint32_t with unsigned char and unsigned int. (This is also the main reason why libdivecomputer doesn't use those C99 integer types anywhere.)
Wouldn't it actually be better if you either typedef'ed all the needed types yourself in a stdint-for-msvc.h ?
Or even, started using <cstdint> ?
This is what I did as a quick hack in aes.h:
#ifdef _MSC_VER typedef unsigned char uint8_t; typedef unsigned int uint32_t; #else #include <stdint.h> #endif
That was enough to get msvc to compile the aes code.
The second one are the global variables. This is not thread-safe, so this will need to be refactored to move those global variables into some aes_ctx_t structure, and pass that to the helper functions instead. The alternative is to link against some crypto library (e.g. openssl). But that might be overkill for what we need.
Its a simpe thing to switch to openssl/aes.h and AES_encrypt()/AES_decrypt() but then Debian and co will go mental about linking gpl software to openssl...
Yea, i know its a bridge to pass, but do firmware code _really_ need to be thread safe?
Late last night i took a stab at, and did a quite quick'n'dirty change to tiny-AES128-C to move the static state varables to a state struct and allocate that one on the stack instead.
I suggested the idea to upstream and we will se what they say, but they aren't completely against the idea:
https://github.com/kokke/tiny-AES128-C/pull/9
Or we could always carry such code as a local patch.
If this gets accepted, then great. If not, we can indeed carry a local patch.
Thread safety is important to me. I'm aware that it's very unlikely that someone will be doing two firmware updates in parallel. But for other use-cases using multiple threads is not uncommon. For example a typical scenario is to download the dives in a worker thread, and parse them on the main (GUI) thread. To make that work, things need to be thread-safe.
If some parts are thread-safe but others are not, that's just asking for trouble. That's why I insist on using only thread-safe code in libdivecomputer.
Jef
-
Anton Lundin -
Jef Driesen