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

updated libraries folder #195

Merged
merged 6 commits into from
Aug 8, 2022
Merged

Conversation

IFX-Anusha
Copy link
Contributor

By creating this pull request you agree to the terms in CONTRIBUTING.md.
https://github.com/Infineon/.github/blob/master/CONTRIBUTING.md
--- DO NOT DELETE ANYTHING ABOVE THIS LINE ---

CONTRIBUTING.md also tells you what to expect in the PR process.

Description
Provide the information we need to review your PR. What problem does the pull request solve? "Bug fix" is not a good description.

Related Issue
If you opened an issue before creating the PR, point to it here.

Context
What do we need to know about your development environment, tools, target, and so on. Screenshots are always helpful if there is a UI element to this PR.

Copy link
Contributor

@techpaul techpaul left a comment

Choose a reason for hiding this comment

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

This appears to me a MASS deletion of libraries YOU have NOT used

Many are for benefit of others and one you have deleted would make YOUR examples board AGNOSTIC, namely LED, which means an example can use LED.Add( pin) and LED.On or LED.Off or LED.Toggle to ALWAYS work on any board whatever method of On being 1 or 0.

This is determined by define XMC_LED_ON in pins_arduino.h
This may break other examples and libraries.

Highlly suspicious of how this PR has been done

Copy link
Contributor

@techpaul techpaul left a comment

Choose a reason for hiding this comment

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

Most RTC functions are supported on XMC1 series AS WELL

Hibernate support was dropped three years ago as Arduino framework does not support detection of Reset from hibernate and staorage to restore system state (including ALL buffers)

@@ -107,46 +107,6 @@ XMC_SPI_t XMC_SPI_0 =
}
};

#elif defined(XMC1400_Arduino_Kit)
Copy link
Member

Choose a reason for hiding this comment

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

Mmm ... the fork has not been updated to the latest develop-2.x branch. And the board is removed from the built-in library. I believe this is one of the boards to keep in 2.x.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it should stay as the XMC1400 will be the successor or the XMC1100

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Will update that

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like internal Infineon converstaions and dreams on a public repo

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully with better layout, documentation.

Maybe also allow other versions to be added like using XMC1400 64 pin variant, I am background working on.

@@ -60,10 +60,6 @@ extern XMC_SPI_t XMC_SPI_0;
#define NUM_SPI 1
extern XMC_SPI_t XMC_SPI_0;

#elif defined(XMC1400_Arduino_Kit)
Copy link
Member

Choose a reason for hiding this comment

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

Same as xmc_spi_conf.c

@@ -197,40 +197,6 @@ XMC_I2C_t XMC_I2C_0 =
.protocol_irq_service_request = 5
};

#elif defined(XMC1400_Arduino_Kit)
Copy link
Member

Choose a reason for hiding this comment

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

Same as xmc_spi_conf.c

@@ -64,10 +64,6 @@ extern XMC_I2C_t XMC_I2C_1;
#define NUM_I2C 1
extern XMC_I2C_t XMC_I2C_0;

#elif defined(XMC1400_Arduino_Kit)
Copy link
Member

Choose a reason for hiding this comment

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

Same as xmc_spi_conf.c

@OlafFilies OlafFilies self-requested a review July 28, 2022 08:48
@jaenrig-ifx
Copy link
Member

@techpaul this develop-2.x is a minimal implementation of xmc-for-arduino.
The current develop/master (v1.x) keeps all the built-in libs.

Only some relevant boards and built-in libraries building and functionally evaluated will be kept.
Many of them will be added later (again) as they are functionally verified and provided testing examples which can be automated and skipped specific hardware dependencies like external sensors, etc.

@techpaul
Copy link
Contributor

@jaenrig-ifx

@techpaul this develop-2.x is a minimal implementation of xmc-for-arduino. The current develop/master (v1.x) keeps all the built-in libs.

Lack of overall description and any details, sounds like an internal test that should have been a separate internal FORK first.

Only some relevant boards and built-in libraries building and functionally evaluated will be kept. Many of them will be added later (again) as they are functionally verified and provided testing examples which can be automated and skipped specific hardware dependencies like external sensors, etc.

So so automated testing and inability to catch the mistake in the same device SPI test, with no external sensors

@techpaul
Copy link
Contributor

A better rework would be to move the xmc_pi_conf and xmc_i2c_conf files to variants folder, for easier maintainability. Note this makes NO difference to compilation paths if in same folder as pins_arduino.h

Copy link
Contributor

@techpaul techpaul left a comment

Choose a reason for hiding this comment

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

Remove functionality for Hiberante examples

@@ -0,0 +1,6 @@
# RTC Library for XMC

This is the real time clock library that is supported by XMC4700
Copy link
Contributor

Choose a reason for hiding this comment

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

XMC1 series as well as XMC4 EXCEPT Hibernate which was dropped from Device control 3 years ago

int i = 0;
void setup()
{
pinMode(LED1, OUTPUT); // setting the LED pin as output
Copy link
Contributor

Choose a reason for hiding this comment

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

1 use LED library to be board agnostic about LED on is high or low
2 Use LED2 as LED 1 is also SPI-SCK on most boards, so could have other disturbances to your SPI (or no effect)

Either you are seeing the HARDWIRED effect of LED1 (BUILTIN_LED) and the LED code is pointless or some other effect is happening

Using LED library and LED2 will meant this will work on nearly every board including XMC2GO

Copy link
Contributor

Choose a reason for hiding this comment

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

Lack of comments at start to say what it does how to configure even hardware makes this a poor example/test "feature"

}
void loop()
{
digitalWrite(LED1, HIGH);
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comments on LED

@@ -107,46 +107,6 @@ XMC_SPI_t XMC_SPI_0 =
}
};

#elif defined(XMC1400_Arduino_Kit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like internal Infineon converstaions and dreams on a public repo

@@ -107,46 +107,6 @@ XMC_SPI_t XMC_SPI_0 =
}
};

#elif defined(XMC1400_Arduino_Kit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully with better layout, documentation.

Maybe also allow other versions to be added like using XMC1400 64 pin variant, I am background working on.

@@ -0,0 +1,62 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me explain this another way, this example has been deleted from relase version of XMC-for-Arduino something like 3 years ago.

The person who wrote this example did not understand how Hibernate works, when the CPU comes out of hibernate the CPU is RESET apart from hibernate falags. This means the code will run Setup and Loop again

The function alarmMatch() will NEVER be reached.

xmc.enableHibernate was removed from DeviceControl library ages ago this example should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This suggests your base for develop-2.x was based on a VERY OLD version of the package

Copy link
Member

Choose a reason for hiding this comment

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

A better rework would be to move the xmc_pi_conf and xmc_i2c_conf files to variants folder, for easier maintainability. Note this makes NO difference to compilation paths if in same folder as pins_arduino.h

Yes, that will be considering in an upcoming improvement 👍

@jaenrig-ifx
Copy link
Member

Together with the upcoming board clean up 👍

@jaenrig-ifx jaenrig-ifx merged commit d47e49a into Infineon:develop-2.x Aug 8, 2022
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.

None yet

4 participants