bipropellant / bipropellant-protocol

defines and implements a low level protocol shared between a few different hoverboard repositories
MIT License
20 stars 17 forks source link

Separateprotocol #26

Closed p-h-a-i-l closed 5 years ago

p-h-a-i-l commented 5 years ago

separates protocol from global variables. Companion PR: https://github.com/bipropellant/bipropellant-hoverboard-firmware/pull/28

Includes https://github.com/bipropellant/bipropellant-protocol/pull/25:

Moves ASCII protocol dependencies into the main repo. Let's get this repo clear of most deps on the main repo!

And https://github.com/bipropellant/bipropellant-protocol/pull/24

p-h-a-i-l commented 5 years ago

found and eliminated abandoned speedsx struct

// gather two separate speed variables togther,
typedef struct tag_SPEEDS{
    int speedl;
    int speedr;
} SPEEDS;
static SPEEDS speedsx = {0,0};
p-h-a-i-l commented 5 years ago

@btsimonh have a look at the code. What do you think? I'm torn apart: I want to have a prefilled params with known data types to share as a template between projects, but now I need to define structs in the protocol headers..

p-h-a-i-l commented 5 years ago

also please check fn_paramstat_descriptions, I just made it compile. Too late to go into logic now ;)

btsimonh commented 5 years ago

so, english saying: 'you want to have your cake and eat it' :). Yes, we're going to need shared structures between applications - but these are definitely a level up from the actual low level protocol. My feeling is remove all from here... and add all params (except maybe the generic descriptions and protocol related stuff) from the using application.

I'm not sure on how best to share the params between controlling and controlled applications. As your coding indicates, the codes and variable structures will most likely be shared, but the functions are likely to be different. So sharing an initialisation structure is not the best? (although I do like the initialisation structure if it was in the application repo as a method of adding - in fact pls add a function which takes an initialisation structure so we can multiple such structures in the main repo, and add each one - then we can separate the 'features' added easily).

Maybe a single file in the main repo which contains DEFINES for the codes and the shared structures, with redefines for the structures to name them to be more explicitly connected to the code name? e.g.

#define CODE_SET_SPEED 0x03
#pragma pack(push, 4)  // all used data types are 4 byte
typedef struct tag_SPEED_DATA {
    // these get set
    long wanted_speed_mm_per_sec[2];

    // configurations/constants
    int speed_max_power; // max speed in this mode
    int speed_min_power; // minimum speed (to get wheels moving)
    int speed_minimum_speed; // below this, we don't ask it to do anything

    // just so it can be read back
    long speed_diff_mm_per_sec[2];
    long speed_power_demand[2];
} SPEED_DATA;
#pragma pack(pop)
#define CODE_SET_SPEED_STRUCT SPEED_DATA

Then, for the moment, copy this file between the applications which share codes? The application would still choose which codes to add to it's structure.... (maybe it becomes a separate repo in time?)

Ref setParamVariable and setParamHandler - these kind of duplicate the functionality of setParam, once you are setting all from the application. Personally, I'd remove both and if required, add a to the application layer; you could add a getParam fn to facilitate modification?

note bug in my original fn_paramstat_descriptions (else it relies on it being item 0, which it will not be now, but was previously, so bug did not cause a visible issue) params[0]->len = 0; -> param->len = 0; (it may be that I should return the first and zero count instead... but later)

p-h-a-i-l commented 5 years ago

Maybe having to update the submodule is a good reminder that changing a struct which is used in the protocol is a breaking change. We could add "old" params in the initialization array and newer ones (in active development) only by the application. But how to avoid reuse of codes?

As a weak protection when updating the variable address and function at runtime, we could check if the length of the variable is the same as the old one..

btsimonh commented 5 years ago

But how to avoid reuse of codes? I was thinking an enum in the main repo. Maybe in the same header which contains the common structures....

something like

enum CODES {
    FLASH_BASE = 0x80,
    FLASH_MAGIC = 0x80,
    FLASH_PKP = 0x81,

    XYT_BASE = 0x0C,
    XYT_READ = 0x0C,
    XYT_SOMMITELSE = 0x0D,
};