T-vK / Electric-Unicycle-Interface

Arduino library to interface electric unicycles (e.g. read speed and temperature or change unicycle's settings) via Serial or Bluetooth
20 stars 3 forks source link

Supporting different EUC models #3

Open voltangle opened 4 years ago

voltangle commented 4 years ago

Just a continuation of #2 issue So, I made a fork, and I am now working on the new design of the library. And also, you need to publish even one release, so everybody can know that this library can be used in production

voltangle commented 4 years ago

Also a new suggestion, about the name of the repo. I think it will be good if named as "EUCSerialInterface". Pretty self-descriptive, and also beautiful :)

And what IDE you have used? I'm just curious)

T-vK commented 4 years ago

I have used the standard Arduino IDE. But as a fan of vim, I've wanted to try out vim-arduino for a while. I'm currently waiting for a bug to get fixed though.

I'd prefer to keep the repository name as is to ensure that links don't break and to avoid confusion.

Do you already have a vision for the new library design? I'd be interested in hearing how you'll approach this.

voltangle commented 4 years ago

I don't have a vision, but I am now prototyping. And about the link, you can rename this repo, create new with old name, and add link to new repo in readme. And about IDE. I use VSCode to make my project. For small projects Arduino IDE is good, but for big projects it is bad. I can suggest you to use VSCode for development. If you want, I can help you migrate

T-vK commented 4 years ago

VSCode is probably really nice, but I don't trust it because Microsoft has a horrible reputation regarding spying on their users using system backdoors and secretly enabling telemetry over and over again even if you have disabled it.

voltangle commented 4 years ago

Yea, that is true. But trust me, there is no telemetry in VSCode

T-vK commented 4 years ago

Even Microsoft would disagree https://code.visualstudio.com/docs/getstarted/telemetry

voltangle commented 4 years ago

Oh. That is bad, sorry for misinformation. But you are free to use Vim + Arduino, it was just my suggestion :) And, do I need to add .vscode directory to .gitignore? Or I can leave it?

T-vK commented 4 years ago

I'd like to keep the repository clean. So it would be nice if you could add .vscode to the gitignore.

voltangle commented 4 years ago

Okay, I will add it to .gitignore

voltangle commented 4 years ago

For now I will be learning how to actually make libraries, and make the actual design of the new library

T-vK commented 4 years ago

Are you talking about C++ libraries in general or Arduino libraries? An Arduino library really just is a zip file containing a C++ library (for example a .h and a .cpp file), a keywords.txt, library.properties file and an examples folder containing example sketches. This project is a pretty clean example of that already. The C++ library would simply be the .h and .cpp file of this project. It could of course have more than one pair of .h/.cpp files, ideally with one class per file. All these classes can then be used in sketches simply by using #include.

voltangle commented 4 years ago

Yes, I am about the Arduino libraries. And also, we need to know that I am not a cpp guy, I am a Kotlin developer, so I don't know much about cpp. For now I am learning cpp basics, so I can build a good library

T-vK commented 4 years ago

You might wanna focus on structs, initialization lists, pointers and #include guards, as I guess these don't exist in Kotlin. Also arrays are much more primitive and more complicated to use in C++.

voltangle commented 4 years ago

Thanks for the guidance! I will learn those parts more precisely :)

voltangle commented 4 years ago

Hi, I have a question. How gotway unicycle sends data through physical rx/tx: byte after byte, or as a packet? I mean, just a string of characters

T-vK commented 4 years ago

A packet is just a series of bytes, so it depends on how you look at it. It's definitely not a a string though because it contains prenty of non-printable characters. The old Gotways that I checked sent 5 packets per second if I recall correnctly. So there was a pause of 200ms between each packet.

voltangle commented 4 years ago

Question about the library header file. What is this part of code?

#if (ARDUINO >= 100)
 #include <Arduino.h>
#else
 #include <WProgram.h>
 #include <pins_arduino.h>
#endif
voltangle commented 4 years ago

And also another question. Why did you use struct, if you can just use class? They are basically the same, but they have some diffs in access by default. So why not use just class?

voltangle commented 4 years ago

And I also have designed the basic organisation of code: isoflow-2

voltangle commented 4 years ago

Still not the one I wanted, I will later post here the final version

T-vK commented 4 years ago

It's a convention thing. A class may be able to achieve the same goal, but C++ people seem to prefer structs for things like that.

If you want to access built in Arduino functions like millis() from a library, you have to #include the header file for that. ARDUINO is the IDE version. If it's >=1.0.0 you must include Arduino.h otherwise, WProgram.h and pins_arduino.h ....

You might have to define completely different callback signatures for different unicycle models that provide more, less or different data.

I think it would be better to have one Euc class and then another class for each unicycle and let them all inherit from Euc. This way you also save some precious memory because there is no need for strings in the constructor anymore. All classes that aren't being used get optimized away during compiling anyway. String should really be avoided if possible when you only have 2k of RAM available. Even an empty string already take 6 bytes.

Maybe for the callback there should only be one parameter representing its own datatype and that datatype could be defined individually for every subclass. That datatype could be a class with a few properties like voltage and milage and a few methods like calculateAcceleration(), calculatePowerUsage() etc.

voltangle commented 4 years ago

I think it needs to look like this:

class GWMSX: public EUC { // inherit attributes and functions from EUC class
    // code
};
voltangle commented 4 years ago

And I have another suggestion in design. We can have special classes for decoding data from unicycles, for example GotWayDecoder(as It is in my diagram). And classes for each unicycle can use that code to decode data.

voltangle commented 4 years ago

I have done the basic design to showcase my vision. I will be glad to see your suggestions

https://github.com/T-vK/Electric-Unicycle-Interface/pull/4

T-vK commented 4 years ago

It seems like the properties and methods of GotWayDecoder could just go into GWMSX. I don't see the benefit of having separate decoding classes.

I also want to bring up conventions. this may be opinion-based, but I think it makes most sense: Class names should be in CamelCase, constants and macros in UPPER_CASE, other variables, functions and class members should be camelCase. Abreviations in class names should only have their first letter uppercaser because otherwise it becomes hard to see where a new word starts. Especially in cases where you have two abreviations next to each other. Also I would say Gotway instead of GotWay and use more descriptive class names. GWMSX looks a bit cryptic. Longer class names don't take additional space btw in case that was your worry.

Do you disagree on having only one callback parameter like I described in the previous comment?

voltangle commented 4 years ago

I agree. And here is what we can do with names: You said about CamelCase, which is true standard for classes. And I named GotWay actually in CamelCase)

I agreee with GWMSX looks a bit cryptic. If it does not take up memory, we can name that class like GotWayMSuperX, or GotWayMSX, or GWMSuperX, you choose. I actually see GotWayMSX as my variant, which I will use. What is you opinion?

And yes, I didn't actually understand that statement about callback parameter. Can you tell me more about it?

T-vK commented 4 years ago

It's just weird to say GotWay instead of Gotway. I mean you don't say AmaZon or MicroSoft. But even if you officially have a capital letter within a brand name like in YouTube, I think it should be treated as a single word and only the first letter should be uppercase, so that it is easy to understand that this is one word and not two.

Since the official name seems to be either Gotway MSuper X or Gotway MSX. By convention the class name would be GotwayMsx or GotwayMsuperX. Every word (including brand names) and abbreviation should only have the first letter in upper case. Otherwise you'll run into issues sooner or later. (A classic example would be HTTPURL where it just becomes completely unreadable compared to HttpUrl.)

I meant something like this:

class GotwayMsxData {
public:
  float voltage;
  float speed;
  float tempMileage;
  float current;
  float temperature;
  float mileage;

  float calculateAcceleration();
  float calculatePower();
  bool isNew();
};
class GotwayMsx: public Euc {
public:
  //...
  void (*eucLoop)(GotwayMsxData);
  //...
  void setCallback(void (*eucLoopCallback)(GotwayMsxData));
  //...
};

which you could then used something like this:

#include <SoftwareSerial.h>
#include <GotwayMsx.h>

SoftwareSerial BluetoothSerial(9,10);
GotwayMsx GotwayMsx(BluetoothSerial, BluetoothSerial);

void setup() {
  Serial.begin(250000);
  BluetoothSerial.begin(9600);
  GotwayMsx.setCallback(eucLoop);
}

void loop() {
  GotwayMsx.tick();
}

void eucLoop(GotwayMsxData &data) {
  if (data.isNew()) {
    Serial.print("Voltage: ");       Serial.print(data.voltage);          Serial.println("V");
    Serial.print("Current: ");       Serial.print(data.current);          Serial.println("A");
    Serial.print("Power: ");         Serial.print(data.calculatePower()); Serial.println("Watt");
    Serial.print("Speed: ");         Serial.print(data.speed);            Serial.println("km/h");
    Serial.print("Total mileage: "); Serial.print(data.mileage,3);        Serial.println("km");
    Serial.print("Temp mileage: ");  Serial.print(data.tempMileage,3);    Serial.println("km");
    Serial.print("Temperature: ");   Serial.print(data.temperature);      Serial.println(" deg Celsius");
    Serial.println("");
    Serial.println("");
  }
}
voltangle commented 4 years ago

Thanks for explaining class names, now I will name them correctly.

Thx for showing the design you want to see, I will try to recreate it!) Later I will indicate you that there is an update

And yes, for what unicycle you have originally created this library? I just need to know where to place logic for decoding data from EUC

One more: what about spreading code for decoding data from unicycles to separate files for each unicycle. Will unused code be ignored at the time of linking?

T-vK commented 4 years ago

I'm not sure what all the models where that I tested this on, but one of them was a Gotway MCM2 and one of them was a Gotway M0.

voltangle commented 4 years ago

Thanks! I will place that code right in the file for MCM2. I think M0 is tooo old)

T-vK commented 4 years ago

They're both like 6 years old. There is no such thing as "too" old.

voltangle commented 4 years ago

Oh. I think I don't need to support THAT old unicycles) We consider InMotion V8 as an old unicycle, when it is already 3 years old)

T-vK commented 4 years ago

The code already exists and many people still have older EUCs. I definitely wouldn't want to drop support for these.

One more: what about spreading code for decoding data from unicycles to separate files for each unicycle. Will unused code be ignored at the time of linking?

The compiler optimizes away any unused code, even if it's in the same file.

voltangle commented 4 years ago

The code already exists and many people still have older EUCs. I definitely wouldn't want to drop support for these.

Well, we can leave that code in the library, but not supporting them. I haven't seen any people even with MCM3(but MCM4 is still in use, cuz it is cheap), so I think we cant support them. I am more of a new guy, which drops support for obsolete devices/OS version/anything else.

The compiler optimizes away any unused code, even if it's in the same file.

Thx! I created the folder structure for ease of code, so you don't need to search for needed device in the big file, instead we just search for specific file and and edit it.

T-vK commented 3 years ago

I'm not expecting you to support them as in answering the repository issues, I'm just saying the work for the M0 and MCM2 has been done already and has proven to be quite solid so far, so this is really just a simple one-time job. Since the MCM2 and M0 send the exact same packages, you could just typedef GotwayM0 to be the same as GotwayMcm2 using typedef GotwayMcm2 M0;.

voltangle commented 3 years ago

I have finished the basic work on design. You can see it in my pull request

voltangle commented 3 years ago

Hello, I have a question. How does makeRawDataUsable() function works?

T-vK commented 3 years ago

What exactly do you mean?

eucUsableData.voltage = (float)((eucRawData.voltage[0] << 8) | eucRawData.voltage[1])/100; Just converts/casts the raw voltage byte data into a float and divides it by 100.

For example lets say the unicycles voltage is 72 volts. In that case it would look like this: ((28<<8) | 32) / 100.

((28<<8) | 32) converts the raw voltage bytes 28 and 32 into an integer.

It might be easier to understand what's happening if you look at it in the hexadecimal system: ((0x1C<<8) | 0x20) This would produce the following number: 0x1C20 and if you convert 0x1C20 back into our decimal system you get 7200. Now you just need to divide it by 100 to get 72.

You can probably do the same with unions btw:

typedef union {
  int asInt;
  byte asBytes[2];
} rawVoltage;

rawVoltage eucCentiVoltage;
eucCentiVoltage.asBytes[0] = eucRawData.voltage[0] ;
eucCentiVoltage.asBytes[1] = eucRawData.voltage[1] ;

float eucVoltage = (float)eucCentiVoltage.asInt/100.0

Maybe you can generalize this a bit and put it in a function that you can reuse.

voltangle commented 3 years ago

I mean, how does data from unicycle gets converted to usable ones? I am just working on Veteran Sherman implementation

T-vK commented 3 years ago

What do you mean? That's exactly what I just told you.

voltangle commented 3 years ago

Oh, sorry, I misunderstood you when I read your reply. Now I understand) Sorry)

voltangle commented 3 years ago

Hello,

I thought: "Why not make my fork a standalone project?" So, don't you mind that my repo will be a standalone repository, with mark that this is a fork of your repo?

T-vK commented 3 years ago

I don't mind at all. Go ahead. :)

voltangle commented 3 years ago

Thanks! :D