Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V4 major revision proposal #1431

Closed
wants to merge 4 commits into from

Commits on Aug 12, 2018

  1. Apply code style to all core C++ files

    Fix .clang-format
    mikee47 committed Aug 12, 2018
    Configuration menu
    Copy the full SHA
    e5f34d1 View commit details
    Browse the repository at this point in the history

Commits on Aug 14, 2018

  1. Sming Framework Version 4.0 core changes

    Having found a framework which does things the 'right way' (unlike Arduino),
    there are some global issues which require attention.
    
    Build environment
    
      I develop on Windows using Eclipse. I use linux for Raspberry Pi work but as yet
      I haven't a dedicated Linux machine but at some point I'll set up a virtual machine
      to check builds on Linux.
    
      I haven't investigated the following aspects of Sming:
        Chocolatey
        Automated testing
    
    Checks
    
      Sming libraries build in SSL and non-SSL with no warnings or errors, except in Libraries - some have issues.
      All Sample applications build successfully.
      Load and run a few of the samples - document results.
    
    Testing framework
    
      There does not appear to be a way of testing the framework on a development system
      (linux / Windows, for example).
      I built a simple 'SmingW' library which pulls in as much of the framework as possible
      to build tools and test code using MinGW. It uses alternative headers to deal with
      low-level definitions and platform-specifics but this is pretty minimal. Obviously
      this would be greatly assisted by tweaks to the framework.
    
    Module coupling
    
        Modules should only be dependent upon those actually required to do the task at hand.
        Unwanted dependencies bloat firmware.
    
    Include files
      A .h file uses the minimum #includes required for the definitions in the header (not the source).
      Where type from non-trivial #include is only accessed by reference, consider placing a forward
      reference instead. e.g. if including a member variable HardwareSerial*, just place
      class HardwareSerial at the top rather than #include "HardwareSerial.h"
      Again, with code modules only the minimum number of #includes to be used.
      Avoid too low-level #includes, use <user_config.h> or "WiringFrameWorkIncludes.h" for example.
      (Selecting the right header file is tricky!)
    
      Whilst just including "SmingCore.h" in everything might be easy, anything in SmingCore or
      Services should not be doing this. Such instances have been replaced with targeted includes.
      Same applies to arduino.h, arduinocompat.h, and so on.
      @todo Get a definitive list of non-preferred header files.
    
    Class headers
    
        Trivial code moved into header files, except where it causes dependency or other
        compiler issues. This benefits optimisation of code size and speed and also reduces
        code size and complexity.
    
        One class, one header (and optional source) file. SmingCore/DataStream/DataSourceStream
        is a big example of what not to do; it has been split into separate files.
    
    Visual verification
    
        Code should be as simple as possible so it can be easily understood by visual inspection.
        It otherwise hides bugs and other problems.
    
    Class member visibility
    
        Member variables should not be declared as public. Instead, use accessor methods.
        There was a lot of this in the HttpConnection, HttpServerConnection, SmtpClient, TcpClient,
        HttpResponse and HttpRequest classes which has been eliminated. Instead, methods have
        been added to operate on the member data instead so code has been moved around. This
        has also eliminated some duplication.
    
    Class variables
    
        These have all been renamed with leading underscore to differentiate from parameters. It is
        way to easy to confuse these with parameters.
    
        Assign default values to all class variables. Makes constructor code simpler and reduces
        change of un-initialised values.
    
    Naming conventions
    
        Names are all camel case. Variables start with a lower case, types with an upper case.
    
    Libraries / Services
    
        Some guidelines as to where modules should be placed. It seems that Libraries are for Arduino
        code, whereas Services are for customised or custom Sming-specific code that doesn't fit in SmingCore.
    
    Libraries
    
        Code quality is hugely variable. There should be two library folders, one for high quality
        code and another for everything which probably needs some attention. Makefiles should
        be updated so user code isn't broken but if I wish to build Sming with only the good
        libraries then it makes that much easier.
    
        ArduinoJson is an example of a high quality module. I note it was recently moved from
        Services; I would consider it a core library. Similarly,
    
        Attributes for a HQ module would include:
          Proven track record
          Written to a good standard (even if it's not the same as the Sming standard)
          No timer-driven polling; code should execute asynchronously using callbacks to perform specific
          tasks, except where alternatives are not available. For example, polling for keypresses via
          SPI or I2C might be necessary if there is no interrupt line available.
          No delay loops or calls to watchdog timer: both of these violate the core principle of Sming.
    
        With compiler warnings turned on, Many of the libraries fail to build; several important warnings
        need to be turned off.
    
        Perhaps 'unapproved' libraries should be built into a separate library, against libsming.
    
    Private functions/data
    
        Where functions and data are meant only for use within a module they must be declared static.
        This avoids unintended conflicts with other modules.
    
    Global class instances
    
        These should be at the top of a source file with other variables, not the bottom.
        The generally accepted ordering is:
    
          #includes
          constant data
          type definitions
          variable data
          source code
    
    Compiler warnings
    
        Code must build with all compiler warnings enabled, and treated as errors, except for:
    
          ... @todo - itemise these
    
    Signed/unsigned
    
        Do not mix int with unsigned values unless required. Compiler will warn where this has been done.
        Most instances have been changed to use unsigned (or size_t) values.
        This is very important. There is an example in m_printf.cpp around line 179:
    
            size_t len;
            int precision = -1;
            ...
            if (len > precision)
    
            Should be:
              if (precision >= 0 && (int)len > precision)
    
            It could be:
              if (len > (unsigned)precision)
            or
              if ((int)len > precision)
    
            The former works, and is the intended action. The latter crashes the CPU because it sets len to -1, or 0xFFFFFFFF.
    
    Character values
    
        Use of char 0 replaced with '\0' (semantics).
    
    Strings
    
        Number of instances where String parameters are passed instead of 'const String&'.
    
        Instead of subString(), remove() is preferred to avoid un-necessary heap allocation and copy.
          e.g. s.remove(0, 1) is equivalent to s = s.subString(1)
    
        There are instances where potentially large data is excessively buffered in order to get it into a String.
        String::setLength() has been added to size the string buffer and allow data to be written into it directly,
        for example via fileRead().  The String class has also been modified internally to use memmove() instead of
        strcpy() operations. This allows a String to contain any kind of data, not just nul-terminated strings.
    
        Where "" is returned for a string value, this frequently means 'value not provided' or 'unknown'. In these
        situations nullptr is more appropriate to indicate 'undefined'. It also allows use of the 'if (str)' expression which
        will evaluate to false. The default constructor for String has not been changed, but perhaps it should - it would
        be the appropriate default value - but it may break existing code.
    
        Two const static data members have been added to String so they may be used where a const reference is required.
        i.e. const String& return values.
    
        NOTE: I may add in a couple of methods for SZ string parsing; they're rather handy.
    
    String and constant data
    
        All text and constant data expliclitly marked with PROGMEM and accessed using appropriate
        macros or functions. This is good practice for a resource-limited system since it gives
        greater control over code generation. If text requires translation into other languages
        this can be automated more easily if appropriate macros are used. It also allows the
        framework to be used with AVR processors if required.
    
        Some additional macros and methods have been added to debug_progmem.h and WString.h to assist
        with this. These are similar to those in Arduino. The main ones are _F() and F(). They both
        define a flash string in-situ and pull it into a buffer. _F() uses a buffer on the stack,
        whereas F() uses a String object so the data is on the heap. The former is preferable
        for functions requiring a C-type string.
    
        The compiler can usually reduce multiple uses of the same constant data into a single instance,
        however this doesn't work so well with these macros.
        Where strings and other constant data are used multiple times it is good practice to define these
        at the top of the source file and refer to them, rather than placing them directly into the code.
        It is preferable to do this for all string data.
    
        The technique I've adopted is to define these strings as function calls. Two macros are used to do this:
    
          DEFINE_STRING_P(_name, _str)   Defines a function which returns a String object with the string data
          DECLARE_STRING_P(_name)        Declares a prototype for the function, used in header files
    
        PROGMEM
    
          A feature of the ESP8266 is the need to access flash memory in aligned 32-bit chunks. The
          AVR processors using non-harvard architecture access code and data on separate busses so
          require the well-documented PROGMEM feature to store this. The ESP8266 has it's fakepgmspace
          module to do a similar job but place such definitions into a read-only flash segment to avoid
          having them copied into RAM at startup.
    
          There is the mforce-l32 GCC compiler mod. to enforce the correct instruction usage, that doesn't seem
          to be available for the standard xtensa toolkit and requires a rebuild. I also recall this
          conflicts with other compiler mods.
    
    Lists and enumeration
    
        There  are instances where a C++ wrapper is required around existing data structures. There are at least two
        instances where a Vector is not the best way to achieve this:
          1. FileSystem.cpp - used to create a directory listing
          2. Station.cpp - used in a callback with an AP list
    
        In (1) using the standard opendir(), readdir(), closedir() is simple enough and far more efficient, plus it
        provides all the required information.
        In (2) we only need a single object to wrap the structure returned by the system. Methods interpret the
        data only as needed, with reset() and next() methods. Additional methods can be provided to take static
        copies of the information, if required, either individually or for the entire data set. There is an
        opportunity here to introduce a standard 'enumerator' class for these sorts of things.
    
    IFS
    
        FileSystem.h modified to remove SPIFFS dependency. Services/SpiFFS changed from .c to .cpp and implements
        spiffs_mount() using IFS, so existing applications work as expected.
    
    Use of auto
    
        This is an awesome C++ feature. When used judiciously it makes code much more readable and reduces the need for
        debugging variable types. It's usually obvious from context what the variable type is so saves some typing as well.
    
    Commented-out code
    
        Removed. Or, if appropriate replaced with a conditional debug expression and/or commented to explain why it's there.
    
    Use of 'friend'
    
        Refactor and remove where appropriate. This violates class encapsulation and makes code less robust. It's really only
        appropriate for small helper classes defined in the same module; using this across separate modules is a bad idea.
    
    Missing function prototypes
    
        Espressif SDK has some missing function prototypes which generate warnings. These have been included.
    
    NULL vs nullptr
    
        Usage in C++ code replaced with nullptr.
        Setting pointers to '0' is not acceptable, replaced with nullptr.
    
    IDataSourceStream / ReadWriteStream
    
        The intent of these two classes is clear: IDataSourceStream is only for reading data out. The uses of these in the
        code was quite convoluted in places. See HttpResponse for an example.
    
    Error return checking
    
        To check return values of functions/methods returning negative error codes, use 'if (ret >= 0)' to check for success,
        and 'if (ret < 0)' for failure.
        Expressions such as 'if (ret != -1)' do not catch out-of-range values.
        It may be prudent to define global functions/macros for this e.g. SUCCESS(_x) and FAILED(_x)
    
    Magic numbers
    
        Never return 'magic numbers' from functions; these must always be defined in a public header file with explanation of their
        purpose. Ideally instances of functions returning -1 should be replaced with a  global error value.
    
    unsigned / unsigned int
    
        These are equivalent so 'unsigned' is preferred for brevity. As with 'int' we use these types where the exact size is
        not important to allow the compiler more freedom.
    
    Macros vs. inline functions
    
        In C++ static inline functions are preferable to macros. They offer type checking and can be overloaded or templated.
    
    Use of 'constexpr'
    
        When used with static inline functions, prompts the compiler to resolve all values at compile time or throw an error.
        Helps to eliminate unintentional code bloat and perhaps more useful than 'force inline'.
    
    Use of 'force inline'
    
        Generally not necessary; compiler optimisation settings should ensure these are inlined correctly.
    
    HTTP Parser
    
      Should we be using http_should_keep_alive() function instead of checking for "close" in headers?
    
      Apart from on_headers_complete, there does not appear to be any requirement for specific error values
      returned from callbacks, they just need to be non-zero. The HPE_xxx error code identifies which
      callback failed, but not the specific code.
    
    Default method/function parameters
    
      Propagated default values from header into definition as comments removed.
      This is not helpful, just clutters the code.
    
    Timers
    
      OSTimer class added to wrap OS timer functions. Used in preference to the main Timer class to reduce RAM usage.
    mikee47 committed Aug 14, 2018
    Configuration menu
    Copy the full SHA
    55cab44 View commit details
    Browse the repository at this point in the history

Commits on Aug 15, 2018

  1. Add global task queue added to SystemClass

    mikee47 committed Aug 15, 2018
    Configuration menu
    Copy the full SHA
    16c9592 View commit details
    Browse the repository at this point in the history
  2. Minor changes to address Travis build failures

    Correct wifi_softap_set_station_info() function prototypes in lwip2 patch and NONOS_SDK patch
    Add NmiTimSetFunc prototype to esp_systemapi.h - missing from SDK 1.5.0
    mikee47 committed Aug 15, 2018
    Configuration menu
    Copy the full SHA
    de20bde View commit details
    Browse the repository at this point in the history