Fix compiler warning #18

hukidd · 2016-12-02T09:43:01Z

This changelist has compiled with different configurations(dSight_Sharp_LCD, HDK_20, HDK_20_SVR,HDK_OLED,HDK_Sharp_LCD), the result is OK.
I have only HDK2.0 device, I have used OSVR app to test this changelist. The result is ok.
I have also used Command Prompt in Atmel Studio to run make all in Source Code/Embedded/makefile-build , The result is same with original code, Hex files have been created for dSight_Sharp_LCD, HDK_20,HDK_20_SVR, HDK_OLED and HDK_Sharp_LCD. But there are the below message both in original code and with this changelist.
[HDK_20] Generating HDK_20/command_parts.json
'jq' 不是內部或外部命令、可執行的程式或批次檔。
add_variant.mk:144: recipe for target 'HDK_20/command_parts.json' failed

rpavlik

Thanks for your contribution! Just a few changes requested - you can squash them along with this existing commit into a single commit. Also, please provide verification that make complete works and that the firmware works on HDK 1.x and HDK 2.

FYI - you appear to not have your razerzone email listed in your github account, these commits are showing up as unrecognized author. You probably also want to fill out your GitHub profile a bit more, at least name and affiliation.

rpavlik · 2016-12-02T16:57:15Z

Source code/Embedded/src/DeviceDrivers/BNO070_using_hostif.c

@@ -214,6 +214,8 @@ static void configureScdFrs(void)
 	}
 }

+//In order to fix compiler warning to mark this function,because this function doesn't been used anywhere
+/*


As a matter of style, commenting out blocks of code is preferred to be done with #if 0 if the code absolutely must remain in the codebase, because it is nestable, doesn't interfere with existing code comments, and is easy to grep for. However, version control keeps old code around for reference, so prefer removing such code entirely.

Does it build with make complete? I don't know if this code was used in the BNO DFU Case.

@OSVR/hillcrest can you take a look at this and offer a review?

Ok, I will remove this function.
I have used Command Prompt in Atmel Studio to run make all in Source Code/Embedded/makefile-build , The result is same with original code, Hex files have been created for dSight_Sharp_LCD, HDK_20,HDK_20_SVR, HDK_OLED and HDK_Sharp_LCD. But there are the below message both in original code and with this changelist.
[HDK_20] Generating HDK_20/command_parts.json
'jq' 不是內部或外部命令、可執行的程式或批次檔。
add_variant.mk:144: recipe for target 'HDK_20/command_parts.json' failed

How to squash them along with this existing commit into a single commit? I use rebase command, it only could squash them in local, it couldn't change them in github. I think it is better that using squash merged method when merging this pull request.

You'd need to "force push" to github in updating this branch. The squash merge method is distinct from cleaning up a pull request by squashing commits.

I only speak English and Spanish, so the error message isn't readble to me, sorry 😄 but my guess is that you don't have "jq" version 1.5 installed which is required for that part. I'll make an issue to update the docs on this point. #20

Thanks for your suggestion to use "force push". I have squashed them along with this existing commit into a single commit

Can you remove this section as you said you would? I think we'd be ready to merge then.

rpavlik · 2016-12-02T16:58:03Z

Source code/Embedded/src/DeviceDrivers/BNO070_using_hostif.c

@@ -611,8 +613,8 @@ bool Tare_BNO070(void)
 {
 	// execute tare commands

-	// In HID parlance, this is a write to the command register, where the command is �set output report.� The �07� byte


Not sure what this diff hunk is, it doesn't look to change anything - except maybe byte values of the numbers? - in diff view on GitHub. Recommend using git add -p or in git gui, selecting and right-click, "stage selection" or "stage hunk" to make diffs minimal.

Nevermind, I see it probably was a byte value/encoding issue and that it was a separate commit. While the advice on selective diff staging still holds, no need to do anything about this change or comment now 😃

rpavlik · 2016-12-02T16:59:18Z

Source code/Embedded/src/DeviceDrivers/bno-hostif/src/sensorhub.c

@@ -1001,11 +1006,6 @@ void avr_readBuf(uint8_t *buf, unsigned long length,
 		 unsigned long index)
 {
    int n = 0;
-	static int trap = 0;
-
-	if(index == 63949) {


What was the original purpose of this code and why can it be removed now?

(Perhaps @OSVR/hillcrest can offer some insight here?)

From code logic, we could see when index =63949 to set trap =1, but trap variable doesn't been used in other place. So it is ok to remove these code.

rpavlik · 2016-12-02T17:00:23Z

Source code/Embedded/src/main.c

@@ -72,6 +72,7 @@ bool HDMI_task = false;
 bool HDMISwitch_task = true;

 void HandleHDMI(void);
+void load_configuration(void);

 void load_configuration(void)


we should just be able to make load_configuration static and avoid the need for a previous declaration, I think.

You can just add static here and not previously declare, and still be warning-less, I think. Please try this.

rpavlik · 2016-12-02T17:02:49Z

Source code/Embedded/src/SerialStateMachine.c

@@ -123,9 +123,11 @@ typedef struct
 static EEPROM_type EEPROM;

 // todo: is this required?
+#ifdef SVR_USING_NXP


Good catch - would be even better if the scope of these variables could be reduced, but this is helpful nonetheless.

rpavlik · 2016-12-02T17:51:14Z

Source code/Embedded/src/ASF/common/services/usb/class/cdc/device/udi_cdc.c

@@ -372,7 +372,8 @@ void udi_cdc_comm_disable(void)
 void udi_cdc_data_disable(void)
 {
    uint8_t port;
-
+    UNUSED(port);


We try to avoid/minimize changes made to vendored files, including ASF - was this change made in a newer upstream version of this file? (Same question applies for the other ASF change.)

We could see the below udi_cdc_line_coding_received( ) function also use "UNUSED(port);" to avoid compiler warning, so it is ok.
static void udi_cdc_line_coding_received(void)
{
uint8_t port = udi_cdc_setup_to_port();
UNUSED(port);

UDI_CDC_SET_CODING_EXT(port, (&udi_cdc_line_coding[port]));

}

rpavlik

Just a few minor changes remain for merge.

rpavlik · 2017-01-17T21:51:38Z

Source code/Embedded/src/DeviceDrivers/BNO070_using_hostif.c

@@ -214,6 +214,8 @@ static void configureScdFrs(void)
 	}
 }

+//In order to fix compiler warning to mark this function,because this function doesn't been used anywhere
+/*


Can you remove this section as you said you would? I think we'd be ready to merge then.

rpavlik · 2017-01-17T21:53:56Z

Source code/Embedded/src/main.c

@@ -72,6 +72,7 @@ bool HDMI_task = false;
 bool HDMISwitch_task = true;

 void HandleHDMI(void);
+void load_configuration(void);

 void load_configuration(void)


You can just add static here and not previously declare, and still be warning-less, I think. Please try this.

rpavlik · 2017-01-18T00:02:51Z

Source code/Embedded/src/DeviceDrivers/BNO070_using_hostif.c

@@ -611,8 +613,8 @@ bool Tare_BNO070(void)
 {
 	// execute tare commands

-	// In HID parlance, this is a write to the command register, where the command is �set output report.� The �07� byte


These two lines of the change also need to be reverted - looks like it changes from ASCII to another encoding for the comments.

rpavlik · 2017-02-01T21:27:53Z

Looks good, thank you!

rpavlik requested changes Dec 2, 2016

View reviewed changes

rpavlik reviewed Dec 2, 2016

View reviewed changes

Fix compiler warning

acea290

hukidd force-pushed the master branch from 15245a2 to acea290 Compare December 7, 2016 01:59

rpavlik requested changes Jan 17, 2017

View reviewed changes

rpavlik reviewed Jan 18, 2017

View reviewed changes

Remove clearScdFrs() and add static in load_configuration()

feab6e0

rpavlik approved these changes Feb 1, 2017

View reviewed changes

rpavlik merged commit 67b0785 into OSVR:master Feb 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compiler warning #18

Fix compiler warning #18

hukidd commented Dec 2, 2016 •

edited

Loading

rpavlik left a comment •

edited

Loading

rpavlik Dec 2, 2016

rpavlik Dec 2, 2016

hukidd Dec 5, 2016

hukidd Dec 5, 2016

rpavlik Dec 6, 2016 •

edited

Loading

hukidd Dec 13, 2016

rpavlik Jan 17, 2017

rpavlik Dec 2, 2016

rpavlik Dec 2, 2016

rpavlik Dec 2, 2016 •

edited

Loading

hukidd Dec 5, 2016

rpavlik Dec 2, 2016

rpavlik Jan 17, 2017

rpavlik Dec 2, 2016

rpavlik Dec 2, 2016

hukidd Dec 5, 2016

rpavlik left a comment

rpavlik Jan 17, 2017

rpavlik Jan 17, 2017

rpavlik Jan 18, 2017

rpavlik commented Feb 1, 2017

Fix compiler warning #18

Fix compiler warning #18

Conversation

hukidd commented Dec 2, 2016 • edited Loading

rpavlik 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

rpavlik Dec 6, 2016 • 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

rpavlik Dec 2, 2016 • 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

rpavlik left a 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

rpavlik commented Feb 1, 2017

hukidd commented Dec 2, 2016 •

edited

Loading

rpavlik left a comment •

edited

Loading

rpavlik Dec 6, 2016 •

edited

Loading

rpavlik Dec 2, 2016 •

edited

Loading