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

Zipping compiler warning about POW_PD_EXT / Two options #1711

Merged
merged 6 commits into from
Jun 20, 2023
Merged

Conversation

ia
Copy link
Collaborator

@ia ia commented Jun 17, 2023

  • There is a warning related to POW_PD_EXT:
In file included from Core/Threads/OperatingModes/ShowStartupWarnings.cpp:1:
./Core/Drivers/HUB238.hpp:6:5: warning: "POW_PD_EXT" is not defined, evaluates to 0 [-Wundef]
    6 | #if POW_PD_EXT == 1
      |     ^~~~~~~~~~
Core/Threads/OperatingModes/ShowStartupWarnings.cpp:40:5: warning: "POW_PD_EXT" is not defined, evaluates to 0 [-Wundef]
   40 | #if POW_PD_EXT == 1
      |     ^~~~~~~~~~
In file included from Core/Threads/OperatingModes/USBPDDebug_HUSB238.cpp:1:
./Core/Drivers/HUB238.hpp:6:5: warning: "POW_PD_EXT" is not defined, evaluates to 0 [-Wundef]
    6 | #if POW_PD_EXT == 1
      |     ^~~~~~~~~~
Core/Threads/OperatingModes/USBPDDebug_HUSB238.cpp:3:5: warning: "POW_PD_EXT" is not defined, evaluates to 0 [-Wundef]
    3 | #if POW_PD_EXT == 1
      |     ^~~~~~~~~~
In file included from Core/Threads/POWThread.cpp:10:
./Core/Drivers/HUB238.hpp:6:5: warning: "POW_PD_EXT" is not defined, evaluates to 0 [-Wundef]
    6 | #if POW_PD_EXT == 1
      |     ^~~~~~~~~~
Core/Threads/POWThread.cpp:62:5: warning: "POW_PD_EXT" is not defined, evaluates to 0 [-Wundef]
   62 | #if POW_PD_EXT == 1
      |     ^~~~~~~~~~
In file included from Core/Drivers/HUB238.cpp:1:
Core/Drivers/HUB238.hpp:6:5: warning: "POW_PD_EXT" is not defined, evaluates to 0 [-Wundef]
    6 | #if POW_PD_EXT == 1
      |     ^~~~~~~~~~
Core/Drivers/HUB238.cpp:6:5: warning: "POW_PD_EXT" is not defined, evaluates to 0 [-Wundef]
    6 | #if POW_PD_EXT == 1
      |     ^~~~~~~~~~
  • I have suggestion with two options:
    • Option A: explicitly add #define POW_PD_EXT 0 to every configuration.h header file.
    • Option B: update #if POW_PD_EXT == 1 to #ifdef POW_PD_EXT.

In my opinion Option B is more preferable ... unless there are some pitfalls and the actual value (0/1) must be checked.

@ia ia changed the title Pow pd ext Zipping compiler warning about POW_PD_EXT / Two options Jun 17, 2023
@Ralim
Copy link
Owner

Ralim commented Jun 18, 2023

Option 1 is preferable; as I suspect that in the future we will have to support other "external" USB-PD controllers (knowing my luck). Just guessing from looking at the market, with people moving to dedicated controllers rather than getting a PD stack working.

#else
#define FLASH_LOGOADDR (0x08000000 + (62 * 1024))
#endif
#endif /* TS80P */

#ifdef MODEL_TS101
Copy link
Collaborator Author

@ia ia Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't you find this way more readable? If no, I will revert it without a question.

Here it doesn't show the idea of modification properly - just in case, for easier review that's what I did. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is a fine cleanup

#endif /* TS101 */

#endif /* CONFIGURATION_H_ */
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may not understanding some particularities so correct me if I'm wrong but shouldn't endif for include-guard be in the very end of the file, right?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah thats a mistake

@ia
Copy link
Collaborator Author

ia commented Jun 19, 2023

And shouldn't this line be more like this: start = xTaskGetTickCount(); (since it declared with type few lines above) ?
I can update this line with next commit right here too if so. Currently it gives shadowed declaration warning.

Sorry for pedantry, just trying to help & be useful. The more eyes the better, right?

@Ralim
Copy link
Owner

Ralim commented Jun 20, 2023

Ill fix the oled shadowing later today; would like to keep it out of this PR

@Ralim Ralim enabled auto-merge (squash) June 20, 2023 00:55
@Ralim Ralim merged commit 155cf38 into Ralim:dev Jun 20, 2023
@ia ia deleted the POW_PD_EXT branch June 21, 2023 17:24
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

Successfully merging this pull request may close these issues.

2 participants