Closed xxEoD2242 closed 4 years ago
This needs to be tested with the full system. Confident of my work but never leave it to chance.
Line 245/246:
GPS_CONNECTED = true;
if (GPS_CONNECTED) {
This if block will always execute. If it's always supposed to execute, remove line 245 and the if check. If it's only supposed to execute when the condition is true, remove line 245. Either way, that line needs to go.
There are 3 sensor setup functions: set_up_gps
, set_up_bmp
, and set_up_bno
. Each of them does different things, which make sense if each sensor has a different setup procedure. However, there's also a function check_for_bmp_status
, which does the same thing for the BMP as set_up_bno
does for the BNO. These functions ought to be named consistently.
Corollary to that, since both the BMP and BNO have a connection check with a timeout (in check_for_bmp_status
and set_up_bno
, respectively), these could both be done in the same loop. Should the GPS sensor have a similar connection timeout check?
One of the principles of functional programming is that a function does the exact same thing given the same inputs every time it is called. However, sample_data_and_build_json_packet
does not do this. Its behavior depends on global variables, which it changes the value of.
In effect, this function is being used inside the setup
function to determine whether it should do its job. It's its own validator. This is not ideal.
Instead, this function ought to be broken up into at least 3 different ones:
create_json_packet
sample_sensors
set_sensor_status_flags
send_data
Then setup
can just call the first three, or a single function that calls the first three, instead of calling the One Big Function (TM).
Line 44/45:
double gps_base_altitude = 0; // Memory location for GPS base altitude
double* gps_alti_offset_address = &gps_base_altitude; // Offset for the GPS for launch reference frame
and 49/50:
double base_altitude = 0; // Memory location for the offset for the BMP
double* alti_offset_address = &base_altitude; // Offset for the base altitude for the BMP
The second variable in each of these is the memory location, and the first is the offset. Comments ought to be corrected. Also, not entirely sure why both are needed; I'd appreciate an explanation.
Line 60: // Function definitions
These are function declarations, the definitions are at the bottom.
Lines 56/57:
imu::Vector<3> vaccel; // 3-dimensional Vector acceleration (non-conservative)
imu::Vector<3> laccel; // 3-dimensional Vector acceleration (conservative)
These variable names ought to be more descriptive. For example, accel_nc
and accel_c
would work.
Out of curiosity, would it be possible to split this into multiple scripts? For example, this one could be called main.ino
, and contain just includes, setup
, and loop
. Sensor-specific setups could be contained in their respective scripts. JSON-handling functions could also go in their own script.
This would make it much easier for the next programmer to change and add on to.
Unfortunately, it cannot, as the boot loader for the Teensy will only flash one file unless we wrote a library for each, which is overkill for what this actually does, in my opinion.
Unfortunately, it cannot, as the boot loader for the Teensy will only accept one file unless we wrote a library for each, which is overkill for what this actually does, in my opinion.
I mean, at the top, we have includes, so the main script could include the rest. Is that prohibited?
Shouldn’t be. I’d have to go look at how it includes the binaries but I think we could should just add them in as libraries through the Arduino IDE and call them just like the other ones. I’ll look at it and comment here.
So the real issue with multiple files is that the IDE compiler doesn’t work like a normal GCC compiler and actually compiles code from a different directory. A lot of people have been looking for solutions but the limitations of the IDE are what they are. You can create a library but, again, for what we have it may be overkill unless we move to not doing JSON and want to do custom structures.
Merging for the sake of getting all this into one place for future review/use.
Check this out.