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

Supporting different EUC models #3

Open
voltangle opened this issue Sep 10, 2020 · 44 comments
Open

Supporting different EUC models #3

voltangle opened this issue Sep 10, 2020 · 44 comments

Comments

@voltangle
Copy link

voltangle commented Sep 10, 2020

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
Copy link
Author

voltangle commented Sep 10, 2020

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
Copy link
Owner

T-vK commented Sep 10, 2020

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
Copy link
Author

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
Copy link
Owner

T-vK commented Sep 10, 2020

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
Copy link
Author

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

@T-vK
Copy link
Owner

T-vK commented Sep 10, 2020

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

@voltangle
Copy link
Author

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
Copy link
Owner

T-vK commented Sep 10, 2020

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

@voltangle
Copy link
Author

Okay, I will add it to .gitignore

@voltangle
Copy link
Author

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

@T-vK
Copy link
Owner

T-vK commented Sep 10, 2020

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
Copy link
Author

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
Copy link
Owner

T-vK commented Sep 11, 2020

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
Copy link
Author

voltangle commented Sep 11, 2020

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

@voltangle
Copy link
Author

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
Copy link
Owner

T-vK commented Sep 12, 2020

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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

@voltangle
Copy link
Author

voltangle commented Sep 12, 2020

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

@T-vK
Copy link
Owner

T-vK commented Sep 12, 2020

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
Copy link
Author

voltangle commented Sep 12, 2020

I think it needs to look like this:

class GWMSX: public EUC { // inherit attributes and functions from EUC class
    // code
};

@voltangle
Copy link
Author

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
Copy link
Author

voltangle commented Sep 12, 2020

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

#4

@T-vK
Copy link
Owner

T-vK commented Sep 13, 2020

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
Copy link
Author

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
Copy link
Owner

T-vK commented Sep 14, 2020

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
Copy link
Author

voltangle commented Sep 14, 2020

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
Copy link
Owner

T-vK commented Sep 14, 2020

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
Copy link
Author

voltangle commented Sep 14, 2020

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

@T-vK
Copy link
Owner

T-vK commented Sep 14, 2020

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

@voltangle
Copy link
Author

voltangle commented Sep 15, 2020

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
Copy link
Owner

T-vK commented Sep 15, 2020

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
Copy link
Author

voltangle commented Sep 15, 2020

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
Copy link
Owner

T-vK commented Sep 15, 2020

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
Copy link
Author

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

@voltangle
Copy link
Author

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

@T-vK
Copy link
Owner

T-vK commented Sep 20, 2020

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
Copy link
Author

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

@T-vK
Copy link
Owner

T-vK commented Sep 20, 2020

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

@voltangle
Copy link
Author

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

@voltangle
Copy link
Author

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
Copy link
Owner

T-vK commented Sep 28, 2020

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

@voltangle
Copy link
Author

Thanks! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants