From 6c12bcc32d46db833808383213f57dba444a5f18 Mon Sep 17 00:00:00 2001 From: tx_haggis <13982343+adbancroft@users.noreply.github.com> Date: Wed, 23 Feb 2022 23:11:54 -0600 Subject: [PATCH] Separate table axis I/O conversion from iteration (#767) * Missing include * 1. Add int16_byte class 2. Add axis factory function for int16_byte instances (flyweight pattern) * Separate out axis iteration and I/O conversions * Optimize for size and performance. * Fix unit test --- speeduino/comms.cpp | 10 +++-- speeduino/int16_byte.h | 48 +++++++++++++++++++++ speeduino/int16_ref.h | 55 ----------------------- speeduino/newComms.cpp | 4 +- speeduino/page_crc.cpp | 6 ++- speeduino/pages.cpp | 17 ++++---- speeduino/src/libdivide/libdivide.h | 2 +- speeduino/storage.cpp | 7 ++- speeduino/table3d_axes.cpp | 6 --- speeduino/table3d_axes.h | 40 ++++++----------- speeduino/table3d_axis_io.cpp | 5 +++ speeduino/table3d_axis_io.h | 67 +++++++++++++++++++++++++++++ speeduino/updates.ino | 4 +- test/test_misc/tests_tables.cpp | 4 +- 14 files changed, 166 insertions(+), 109 deletions(-) create mode 100644 speeduino/int16_byte.h delete mode 100644 speeduino/int16_ref.h delete mode 100644 speeduino/table3d_axes.cpp create mode 100644 speeduino/table3d_axis_io.cpp create mode 100644 speeduino/table3d_axis_io.h diff --git a/speeduino/comms.cpp b/speeduino/comms.cpp index 8c4a325b..c138588b 100644 --- a/speeduino/comms.cpp +++ b/speeduino/comms.cpp @@ -18,6 +18,7 @@ A full copy of the license may be found in the projects root directory #include "pages.h" #include "page_crc.h" #include "logger.h" +#include "table3d_axis_io.h" #ifdef RTC_ENABLED #include "rtc_common.h" #endif @@ -793,9 +794,10 @@ namespace { inline void send_table_axis(table_axis_iterator it) { + const int16_byte *pConverter = table3d_axis_io::get_converter(it.domain()); while (!it.at_end()) { - Serial.write((byte)*it); + Serial.write(pConverter->to_byte(*it)); ++it; } } @@ -901,7 +903,7 @@ namespace { void print_row(const table_axis_iterator &y_it, table_row_iterator row) { - serial_print_prepadded_value((byte)*y_it); + serial_print_prepadded_value(table3d_axis_io::to_byte(y_it.domain(), *y_it)); while (!row.at_end()) { @@ -916,9 +918,11 @@ namespace { Serial.print(F(" ")); auto x_it = x_begin(pTable, key); + const int16_byte *pConverter = table3d_axis_io::get_converter(x_it.domain()); + while(!x_it.at_end()) { - serial_print_prepadded_value((byte)*x_it); + serial_print_prepadded_value(pConverter->to_byte(*x_it)); ++x_it; } } diff --git a/speeduino/int16_byte.h b/speeduino/int16_byte.h new file mode 100644 index 00000000..c7c093b0 --- /dev/null +++ b/speeduino/int16_byte.h @@ -0,0 +1,48 @@ +/** + * @addtogroup table_3d + * @{ + */ + +#pragma once + +#include +#include "src/libdivide/libdivide.h" + +/** @brief Byte type. This is not defined in any C or C++ standard header. */ +typedef uint8_t byte; + +/** @brief Represents a 16-bit value as a byte. Useful for I/O. + * + * Often we need to deal internally with values that fit in 16-bits but do + * not require much accuracy. E.g. table axes in RPM. For these values we can + * save storage space (EEPROM) by scaling to/from 8-bits using a fixed divisor. + */ +class int16_byte +{ +public: + + /** + * @brief Construct + * @param factor The factor to multiply when converting \c byte to \c int16_t + * @param divider The factor to divide by when converting \c int16_t to \c byte + * + * \c divider could be computed from \c factor, but including it as a parameter + * allows callers to create \c factor instances at compile tiem. + */ + constexpr int16_byte(uint8_t factor, const libdivide::libdivide_s16_t ÷r) + : _factor(factor), _divider(divider) + { + } + + /** @brief Convert to a \c byte */ + inline byte to_byte(int16_t value) const { return _factor==1 ? value : _factor==2 ? value>>1 : (byte)libdivide::libdivide_s16_do(value, &_divider); } + + /** @brief Convert from a \c byte */ + inline int16_t from_byte( byte in ) const { return (int16_t)in * _factor; } + +private: + uint8_t _factor; + libdivide::libdivide_s16_t _divider; +}; + +/** @} */ \ No newline at end of file diff --git a/speeduino/int16_ref.h b/speeduino/int16_ref.h deleted file mode 100644 index deeb7594..00000000 --- a/speeduino/int16_ref.h +++ /dev/null @@ -1,55 +0,0 @@ -/** - * @addtogroup table_3d - * @{ - */ - -#pragma once - -#include -#include "src/libdivide/libdivide.h" - -/** @brief Byte type. This is not defined in any C or C++ standard header. */ -typedef uint8_t byte; - -/** @brief Represents a 16-bit value as a byte. Useful for I/O. - * - * Often we need to deal internally with values that fit in 16-bits but do - * not require much accuracy. E.g. table axes in RPM. For these values we can - * save storage space (EEPROM) by scaling to/from 8-bits using a fixed divisor. - */ -class int16_ref -{ -public: - -#pragma pack(push, 1) - struct scalar { - uint8_t _factor; - libdivide::libdivide_s16_t divider; - }; -#pragma pack(pop) - - /** - * @brief Construct - * @param value The \c int16_t to encapsulate. - * @param pScalar The factor to scale the \c int16_t value by when converting to/from a \c byte - */ - int16_ref(int16_t &value, const scalar *pScalar) - : _value(value), _pScalar(pScalar) - { - } - - /** @brief Convert to a \c byte */ - inline byte operator*() const { return (byte)libdivide::libdivide_s16_do(_value, &_pScalar->divider); } - - /** @brief Convert to a \c byte */ - inline explicit operator byte() const { return **this; } - - /** @brief Convert from a \c byte */ - inline int16_ref &operator=( byte in ) { _value = (int16_t)in * (int16_t)(_pScalar->_factor); return *this; } - -private: - int16_t &_value; - const scalar *_pScalar; -}; - -/** @} */ diff --git a/speeduino/newComms.cpp b/speeduino/newComms.cpp index f8f7d749..4fc8ba4a 100644 --- a/speeduino/newComms.cpp +++ b/speeduino/newComms.cpp @@ -20,6 +20,7 @@ A full copy of the license may be found in the projects root directory #include "logger.h" #include "comms.h" #include "src/FastCRC/FastCRC.h" +#include "table3d_axis_io.h" #ifdef RTC_ENABLED #include "rtc_common.h" #endif @@ -920,9 +921,10 @@ namespace inline void send_table_axis(table_axis_iterator it) { + const int16_byte *pConverter = table3d_axis_io::get_converter(it.domain()); while (!it.at_end()) { - Serial.write((byte)*it); + Serial.write(pConverter->to_byte(*it)); ++it; } } diff --git a/speeduino/page_crc.cpp b/speeduino/page_crc.cpp index 104c0eeb..d398defd 100644 --- a/speeduino/page_crc.cpp +++ b/speeduino/page_crc.cpp @@ -1,7 +1,7 @@ #include "globals.h" #include "page_crc.h" #include "pages.h" -//#include "src/FastCRC/FastCRC.h" +#include "table3d_axis_io.h" typedef uint32_t (FastCRC32::*pCrcCalc)(const uint8_t *, const uint16_t, bool); @@ -30,11 +30,13 @@ static inline uint32_t compute_tablevalues_crc(table_value_iterator it, pCrcCalc static inline uint32_t compute_tableaxis_crc(table_axis_iterator it, uint32_t crc) { + const int16_byte *pConverter = table3d_axis_io::get_converter(it.domain()); + byte values[32]; // Fingers crossed we don't have a table bigger than 32x32 byte *pValue = values; while (!it.at_end()) { - *pValue++ = (byte)*it; + *pValue++ = pConverter->to_byte(*it); ++it; } return pValue-values==0 ? crc : CRC32.crc32_upd(values, pValue-values, false); diff --git a/speeduino/pages.cpp b/speeduino/pages.cpp index f9b0b9f1..26cdb87b 100644 --- a/speeduino/pages.cpp +++ b/speeduino/pages.cpp @@ -1,6 +1,7 @@ #include "pages.h" #include "globals.h" #include "utilities.h" +#include "table3d_axis_io.h" // Maps from virtual page "addresses" to addresses/bytes of real in memory entities // @@ -79,10 +80,10 @@ public: case table_location_values: return get_value_value(); case table_location_xaxis: - return *get_xaxis_value(); + return table3d_axis_io::to_byte(table_t::xaxis_t::domain, get_xaxis_value()); case table_location_yaxis: default: - return *get_yaxis_value(); + return table3d_axis_io::to_byte(table_t::yaxis_t::domain, get_yaxis_value()); } } @@ -96,12 +97,12 @@ public: break; case table_location_xaxis: - get_xaxis_value() = new_value; + get_xaxis_value() = table3d_axis_io::from_byte(table_t::xaxis_t::domain, new_value); break; case table_location_yaxis: default: - get_yaxis_value() = new_value; + get_yaxis_value() = table3d_axis_io::from_byte(table_t::yaxis_t::domain, new_value); } invalidate_cache(&_pTable->get_value_cache); return *this; @@ -114,14 +115,14 @@ private: return _pTable->values.value_at((uint8_t)_table_offset); } - inline int16_ref get_xaxis_value() const + inline table3d_axis_t& get_xaxis_value() const { - return *_pTable->axisX.begin().advance(_table_offset - get_table_value_end()); + return *(_pTable->axisX.begin().advance(_table_offset - get_table_value_end())); } - inline int16_ref get_yaxis_value() const + inline table3d_axis_t& get_yaxis_value() const { - return *_pTable->axisY.begin().advance(_table_offset - get_table_axisx_end()); + return *(_pTable->axisY.begin().advance(_table_offset - get_table_axisx_end())); } enum table_location { diff --git a/speeduino/src/libdivide/libdivide.h b/speeduino/src/libdivide/libdivide.h index e9a31d15..1f245b82 100644 --- a/speeduino/src/libdivide/libdivide.h +++ b/speeduino/src/libdivide/libdivide.h @@ -16,9 +16,9 @@ #define LIBDIVIDE_VERSION_MINOR 0 #include +#include #if !defined(__AVR__) #include -#include #endif #if defined(LIBDIVIDE_SSE2) diff --git a/speeduino/storage.cpp b/speeduino/storage.cpp index 637b47fd..b07d1fdb 100644 --- a/speeduino/storage.cpp +++ b/speeduino/storage.cpp @@ -11,6 +11,7 @@ A full copy of the license may be found in the projects root directory #include EEPROM_LIB_H //This is defined in the board .h files #include "storage.h" #include "pages.h" +#include "table3d_axis_io.h" //The maximum number of write operations that will be performed in one go. If we try to write to the EEPROM too fast (Each write takes ~3ms) then the rest of the system can hang) #if defined(CORE_STM32) || defined(CORE_TEENSY) & !defined(USE_SPI_EEPROM) @@ -120,9 +121,10 @@ static inline write_location write(table_value_iterator it, write_location locat static inline write_location write(table_axis_iterator it, write_location location) { + const int16_byte *pConverter = table3d_axis_io::get_converter(it.domain()); while ((location.can_write() || forceBurn) && !it.at_end()) { - location.update((byte)*it); + location.update(pConverter->to_byte(*it)); ++location; ++it; } @@ -346,9 +348,10 @@ static inline eeprom_address_t load(table_value_iterator it, eeprom_address_t ad static inline eeprom_address_t load(table_axis_iterator it, eeprom_address_t address) { + const int16_byte *pConverter = table3d_axis_io::get_converter(it.domain()); while (!it.at_end()) { - *it = EEPROM.read(address); + *it = pConverter->from_byte(EEPROM.read(address)); ++address; ++it; } diff --git a/speeduino/table3d_axes.cpp b/speeduino/table3d_axes.cpp deleted file mode 100644 index 1c455c64..00000000 --- a/speeduino/table3d_axes.cpp +++ /dev/null @@ -1,6 +0,0 @@ -#include -#include "table3d_axes.h" - -constexpr int16_ref::scalar table3d_axis_base::scalar_100; -constexpr int16_ref::scalar table3d_axis_base::scalar_2; -constexpr int16_ref::scalar table3d_axis_base::scalar_1; \ No newline at end of file diff --git a/speeduino/table3d_axes.h b/speeduino/table3d_axes.h index 36627784..bc57f00d 100644 --- a/speeduino/table3d_axes.h +++ b/speeduino/table3d_axes.h @@ -10,8 +10,6 @@ #pragma once #include "table3d_typedefs.h" -#include "int16_ref.h" -#include "src/libdivide/constant_fast_div.h" /**\enum axis_domain * @brief Encodes the real world measurement that a table axis captures @@ -31,11 +29,13 @@ class table_axis_iterator public: /** @brief Construct */ - table_axis_iterator(const table3d_axis_t *pStart, const table3d_axis_t *pEnd, const int16_ref::scalar *pScalar, int8_t stride) - : _pAxis(pStart), _pAxisEnd(pEnd), _stride(stride), _pScalar(pScalar) + table_axis_iterator(const table3d_axis_t *pStart, const table3d_axis_t *pEnd, int8_t stride, axis_domain domain) + : _pAxis(pStart), _pAxisEnd(pEnd), _stride(stride), _domain(domain) { } + axis_domain domain(void) const { return _domain; } + /** @brief Advance the iterator * @param steps The number of elements to move the iterator */ @@ -58,14 +58,14 @@ public: } /** @brief Dereference the iterator */ - inline int16_ref operator*() + inline table3d_axis_t& operator*() { - return int16_ref(*const_cast(_pAxis), _pScalar); + return *const_cast(_pAxis); } /** @copydoc table_axis_iterator::operator*() */ - inline const int16_ref operator*() const + inline const table3d_axis_t& operator*() const { - return int16_ref(*const_cast(_pAxis), _pScalar); + return *_pAxis; } /** @brief Reverse the iterator direction @@ -85,28 +85,14 @@ private: const table3d_axis_t *_pAxis; const table3d_axis_t *_pAxisEnd; int8_t _stride; - const int16_ref::scalar *_pScalar; -}; - -/** @brief Shared code for the axis types */ -class table3d_axis_base { -protected: - static constexpr const int16_ref::scalar* domain_to_scalar(axis_domain domain) { - // This really, really needs to be done at compile time, hence the contexpr - return domain==axis_domain_Rpm ? &scalar_100 : - domain==axis_domain_Load ? &scalar_2 : &scalar_1; - } -private: - static constexpr const int16_ref::scalar scalar_100 = { 100, { S16_MAGIC(100), S16_MORE(100) } }; - static constexpr const int16_ref::scalar scalar_2 = { 2, { S16_MAGIC(2), S16_MORE(2) } }; - static constexpr const int16_ref::scalar scalar_1 = { 1, { S16_MAGIC(1), S16_MORE(1) } }; + axis_domain _domain; }; #define TABLE3D_TYPENAME_XAXIS(size, xDom, yDom) CONCAT(TABLE3D_TYPENAME_BASE(size, xDom, yDom), _xaxis) #define TABLE3D_GEN_XAXIS(size, xDom, yDom) \ /** @brief The x-axis for a 3D table with size x size dimensions, xDom x-axis and yDom y-axis */ \ - struct TABLE3D_TYPENAME_XAXIS(size, xDom, yDom) : public table3d_axis_base { \ + struct TABLE3D_TYPENAME_XAXIS(size, xDom, yDom) { \ /** @brief The length of the axis in elements */ \ static constexpr table3d_dim_t length = size; \ /** @brief The domain the axis represents */ \ @@ -120,7 +106,7 @@ private: /** @brief Iterate over the axis elements */ \ inline table_axis_iterator begin() \ { \ - return table_axis_iterator(axis, axis+size, domain_to_scalar(domain), 1); \ + return table_axis_iterator(axis, axis+size, 1, domain); \ } \ }; TABLE3D_GENERATOR(TABLE3D_GEN_XAXIS) @@ -129,7 +115,7 @@ TABLE3D_GENERATOR(TABLE3D_GEN_XAXIS) #define TABLE3D_GEN_YAXIS(size, xDom, yDom) \ /** @brief The y-axis for a 3D table with size x size dimensions, xDom x-axis and yDom y-axis */ \ - struct CONCAT(TABLE3D_TYPENAME_BASE(size, xDom, yDom), _yaxis) : public table3d_axis_base { \ + struct CONCAT(TABLE3D_TYPENAME_BASE(size, xDom, yDom), _yaxis) { \ /** @brief The length of the axis in elements */ \ static constexpr table3d_dim_t length = size; \ /** @brief The domain the axis represents */ \ @@ -143,7 +129,7 @@ TABLE3D_GENERATOR(TABLE3D_GEN_XAXIS) /** @brief Iterate over the axis elements */ \ inline table_axis_iterator begin() \ { \ - return table_axis_iterator(axis+(size-1), axis-1, domain_to_scalar(domain), -1); \ + return table_axis_iterator(axis+(size-1), axis-1, -1, domain); \ } \ }; TABLE3D_GENERATOR(TABLE3D_GEN_YAXIS) diff --git a/speeduino/table3d_axis_io.cpp b/speeduino/table3d_axis_io.cpp new file mode 100644 index 00000000..4ba6502e --- /dev/null +++ b/speeduino/table3d_axis_io.cpp @@ -0,0 +1,5 @@ +#include "table3d_axis_io.h" + +constexpr int16_byte table3d_axis_io::converter_100; +constexpr int16_byte table3d_axis_io::converter_2; +constexpr int16_byte table3d_axis_io::converter_1; \ No newline at end of file diff --git a/speeduino/table3d_axis_io.h b/speeduino/table3d_axis_io.h new file mode 100644 index 00000000..f81de609 --- /dev/null +++ b/speeduino/table3d_axis_io.h @@ -0,0 +1,67 @@ +/** + * @addtogroup table_3d + * @{ + */ + +/** \file + * @brief 3D table axis I/O support + */ + +#pragma once + +#include "int16_byte.h" +#include "table3d_axes.h" +#include "src/libdivide/constant_fast_div.h" + +/** @brief table axis I/O support + * + * @attention Using \c constexpr class static variables seems to be the best + * combination of size and speed - \c constexpr implies \c inline, so we + * can't use it on traditional \c extern global variables. +*/ +class table3d_axis_io { +public: + + /** + * @brief Obtain a converter instance for a given axis domain. + * + * Often we need to deal internally with values that fit in 16-bits but do + * not require much accuracy. E.g. RPM. For these values we can + * save storage space (EEPROM) by scaling to/from 8-bits using a fixed divisor. + * + * The divisor is dependent on the domain. I.e all axes with the same domain use + * the same divisor + * + * Conversion during I/O is orthogonal to other axis concerns, so is separated and + * encapsulated here. + */ + static constexpr const int16_byte* get_converter(axis_domain domain) { + return domain==axis_domain_Rpm ? &converter_100 : + domain==axis_domain_Load ? &converter_2 : &converter_1; + } + + /** + * @brief Convert to a \c byte + * + * Useful for converting a single value. + * If converting multiple, probably faster to cache the converter rather than + * repeatedly calling this function. + */ + static inline byte to_byte(axis_domain domain, int16_t value) { return get_converter(domain)->to_byte(value); } + + /** + * @brief Convert from a \c byte + * + * Useful for converting a single value. + * If converting multiple, probably faster to cache the converter rather than + * repeatedly calling this function. + */ + static inline int16_t from_byte(axis_domain domain, byte in ) { return get_converter(domain)->from_byte(in); } + +private: + static constexpr int16_byte converter_100 = { 100, { S16_MAGIC(100), S16_MORE(100) } }; + static constexpr int16_byte converter_2 = { 2, { S16_MAGIC(2), S16_MORE(2) } }; + static constexpr int16_byte converter_1 = { 1, { S16_MAGIC(1), S16_MORE(1) } }; +}; + +/** @} */ \ No newline at end of file diff --git a/speeduino/updates.ino b/speeduino/updates.ino index c64c9c7f..936d4dcd 100644 --- a/speeduino/updates.ino +++ b/speeduino/updates.ino @@ -622,7 +622,7 @@ void multiplyTableLoad(const void *pTable, table_type_t key, uint8_t multiplier) auto y_it = y_begin(pTable, key); while(!y_it.at_end()) { - *y_it = (byte)*y_it * multiplier; + *y_it = *y_it * multiplier; ++y_it; } } @@ -632,7 +632,7 @@ void divideTableLoad(const void *pTable, table_type_t key, uint8_t divisor) auto y_it = y_begin(pTable, key); while(!y_it.at_end()) { - *y_it = (byte)*y_it / divisor; //Previous TS scale was 2.0, now is 0.5, 4x increase + *y_it = *y_it / divisor; //Previous TS scale was 2.0, now is 0.5, 4x increase ++y_it; } } diff --git a/test/test_misc/tests_tables.cpp b/test/test_misc/tests_tables.cpp index 63cd8a33..10cc9b49 100644 --- a/test/test_misc/tests_tables.cpp +++ b/test/test_misc/tests_tables.cpp @@ -8,9 +8,9 @@ #define _countof(x) (sizeof(x) / sizeof (x[0])) #if defined(PROGMEM) -const PROGMEM byte values[] = { +const PROGMEM table3d_value_t values[] = { #else -const byte values[] = { +const table3d_value_t values[] = { #endif //0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 109, 111, 112, 113, 114, 114, 114, 115, 115, 115, 114, 114, 114, 114, 114, 114,