Fixes and updates #1

bluesign2k · 2017-01-21T17:35:21Z

We want to run the entry setup of the new state, not the previous one :)

Using INPUT_PULLUP as an argument for digitalWrite is risky at best as a method to enable the pullup on a pin (valid parameters are 0 (LOW) or 1 (HIGH). INPUT_PULLUP has a value of 2). Setting pullups should be done thorugh pinMode. See here: https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/Arduino.h https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/wiring_digital.c

Replaced the low level SPI driver for a more generic RX5808 one. This should allow for more flexible control of receivers and later addition of new features, including individual control of receivers, setting arbitrary frequencies, and generally improving code structure.

…y' into Knifa/tidy-up

bluesign2k · 2017-01-22T15:35:42Z

Comments for the last commit didn't propagate over when I merged my branches, not sure why... here it is. I hope they're of interest...
"Replaced the low level SPI driver for a more generic RX5808 one.
This should allow for more flexible control of receivers and later addition of new features, including individual control of receivers, setting arbitrary frequencies, and generally improving code structure."

Knifa · 2017-01-30T17:30:50Z

Woah, thanks for this! Sorry for not checking sooner, GitHub didn't give me a notification for some reason. I'll have a bit of a better look shortly. :)

Knifa · 2017-01-30T17:33:37Z

src/rx5808-pro-diversity/state.cpp

@@ -43,7 +43,7 @@ namespace StateMachine {
        currentHandler = newHandler;

        if (newHandler)
-            lastHandler->onEnter();
+			newHandler->onEnter();


I've fixed this in my most recent push which I think is the only merge conflict. Well spotted! Took me ages to find and fix this myself, haha.

Knifa · 2017-01-30T17:39:46Z

This is awesome, thanks! Let me know when you're ready to merge. This branch is still a WIP for upstream too, so might get a little weird.

Purely cosmetic, but can you aim to match existing conventions? Things like:

Spaces over tabs
camelCase for methods rather than CamelCase
etc.

Happy to go over this myself afterwards either way. Pretty keen on making this is clean and standard as possible so we don't end up in a similar state as the master.

As requested, replaced tabs with spaces and lowercase function names

bluesign2k · 2017-01-30T20:33:18Z

No problem. I've been meaning to do a re-write such as you've done for a while as the upstream has clearly got a bit untidy but it was such a big task that I never really got into it. Now that there's a more sensible base, I'm keen to help with features, testing and tidying etc.

I've just pushed a commit that clears up the spaces/tabs and naming. Let me know if there's anything else you think needs changing. I think I've also cleared up the conflict. I was going to try rebasing off you latest, but tbh, my github skills aren't great.

Knifa

This is really cool over all! Just a couple more convention things, sorry to be super pedantic. First time using GitHub's review thingie as well.

Knifa · 2017-01-31T06:45:53Z

src/rx5808-pro-diversity/RX5808.cpp

+RX5808::RX5808(void) :
+    minRSSI(DEFAULT_MIN_RSSI),
+    maxRSSI(DEFAULT_MAX_RSSI)
+{}


You should be able to set these as defaults directly in the class definition instead rather than using initialisation lists.

Knifa · 2017-01-31T06:46:26Z

src/rx5808-pro-diversity/RX5808.cpp

+}
+
+uint16_t RX5808::getRSSI(uint8_t averages)
+{


Can you put braces on the same line rather than a separate line?

Knifa · 2017-01-31T06:47:14Z

src/rx5808-pro-diversity/RX5808.cpp

+    return (sum / averages);
+}
+
+uint16_t RX5808::setFrequencyMHz(uint16_t frequency)


This is really great! I really wanted to allow manual fine tuning.

Knifa · 2017-01-31T06:48:27Z

src/rx5808-pro-diversity/RX5808.h

+#define RTC6715_POWER_DOWN_CONTROL_REGISTER             0x0A
+#define RTC6715_STATUS_REGISTER                         0x0F
+
+class RX5808


Can you rename the class to Rx5808? Like HtmlClass or EepromSettings.

Knifa · 2017-01-31T06:48:50Z

src/rx5808-pro-diversity/RX5808.h

+    void _chipSelectDeassert(void);
+};
+
+#endif


Can you add new lines to the end of your files?

Knifa · 2017-01-31T06:52:55Z

src/rx5808-pro-diversity/RX5808.h

+
+private:
+    // interface pins
+    uint8_t _pChipSelect;


Can you drop the leading _ from your privates? I am still so-so on this because of conflicting rules about it being reserved or not reserved, but if we could stick without for now that would be cool.

Knifa · 2017-01-31T06:53:18Z

src/rx5808-pro-diversity/receiver.cpp

    #endif

+    RX5808 hReceivers[RECEIVER_COUNT];


What does the h mean?

the 'h' is because it's a handle

Knifa · 2017-01-31T06:53:41Z

src/rx5808-pro-diversity/receiver.cpp

@@ -56,22 +72,10 @@ namespace Receiver {
    uint16_t updateRssi() {
        waitForStableRssi();

-        rssiARaw = 0;


This is way tidier! Love it.

Knifa · 2017-01-31T06:54:30Z

src/rx5808-pro-diversity/rx5808-pro-diversity.ino

@@ -122,6 +122,9 @@ screens drawScreen;

 #endif

+void setupPins();
+void setupSettings();
+void haltWithError();


Thanks for making these explicit. I keep forgetting the Arduino IDE does a bunch of weird stuff to "help" you.

Formatting changes, as requested

bluesign2k · 2017-08-25T13:41:35Z

Only a few months late, but I've now made the requested formatting changes. Because I'm a bit behind, I think there's some conflicts, but they should be pretty simple fixes.

bluesign2k added 4 commits January 21, 2017 17:30

Fix handler setup

6488c7e

Merge remote-tracking branch 'refs/remotes/origin/new-receiver-librar…

db8ac28

…y' into Knifa/tidy-up

bluesign2k changed the title ~~Fix handler setup~~ Fixes and updates Jan 22, 2017

Knifa reviewed Jan 30, 2017

View reviewed changes

Formatting corrections

7879b94

As requested, replaced tabs with spaces and lowercase function names

Merge branch 'tidy-up' into tidy-up

d045aaa

Knifa requested changes Jan 31, 2017

View reviewed changes

Formatting changes

dac5b05

Formatting changes, as requested

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and updates #1

Fixes and updates #1

bluesign2k commented Jan 21, 2017

bluesign2k commented Jan 22, 2017

Knifa commented Jan 30, 2017

Knifa Jan 30, 2017

Knifa commented Jan 30, 2017

bluesign2k commented Jan 30, 2017

Knifa left a comment •

edited

Loading

Knifa Jan 31, 2017

Knifa Jan 31, 2017

Knifa Jan 31, 2017

Knifa Jan 31, 2017

Knifa Jan 31, 2017

Knifa Jan 31, 2017

Knifa Jan 31, 2017

bluesign2k Aug 25, 2017

Knifa Jan 31, 2017

Knifa Jan 31, 2017

bluesign2k commented Aug 25, 2017

Fixes and updates #1

Are you sure you want to change the base?

Fixes and updates #1

Conversation

bluesign2k commented Jan 21, 2017

bluesign2k commented Jan 22, 2017

Knifa commented Jan 30, 2017

Choose a reason for hiding this comment

Knifa commented Jan 30, 2017

bluesign2k commented Jan 30, 2017

Knifa left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluesign2k commented Aug 25, 2017

Knifa left a comment •

edited

Loading