Skip to content

Conversation

nightswimmer
Copy link

This PR adds support to read data from 3 encoders, one from each port of the board.

Older workflows will need a small modification to work with this version. The values from the two analog inputs and the single encoder supported were being reported in a single register with a 3 position array. This new version changes this into a 2 array position for the analog inputs only, and moves the encoder readings into a new 3 position array.

@nightswimmer nightswimmer added the feature New planned feature label Jul 24, 2025
@CLAassistant
Copy link

CLAassistant commented Jul 24, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@bruno-f-cruz bruno-f-cruz left a comment

Choose a reason for hiding this comment

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

I also noticed that EnabledEvents was not changed. This register should have an additional flag to toggle event on and off for consistency (even if the event can be disabled by disabling the encoders register).

{
uint8_t led[3] = {0, 0, 0};

timer_type0_stop(&TCF0); clr_DO0;
Copy link
Member

Choose a reason for hiding this comment

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

Probably unintended change

if (t1ms++ & 1)
{
/* Read ADC */
core_func_mark_user_timestamp();
Copy link
Member

Choose a reason for hiding this comment

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

A fair amount changes seem to be due to different editor configuration. Ideally these would not be tracked in the PR

Copy link
Collaborator

@glopesdev glopesdev Jul 26, 2025

Choose a reason for hiding this comment

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

@bruno-f-cruz good point, probably the best would be to provide a .editorconfig file defining the common rules and then go from there.

@nightswimmer we can look at this together and reproduce the settings for the existing project to minimize the diff, then rebase.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bruno-f-cruz me and @nightswimmer were looking at this together and it looks like the spacing across firmware source code is very much random, even within the same file sometimes tabs are used, sometimes spaces, and it is not very clear which one is the dominant style.

If we are going to correct this, it might be better to just unify the entire source code into a single format (I suggest spaces) and possibly add ignore this formatting commit for git blame.

I will prepare a PR for review just with very basic editor config settings and we can decide.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bruno-f-cruz Actually, let's backtrack on this, it is too messy and ambiguous to understand at this point, probably best to raise an issue and decide a consistent strategy for editor config across all files before we start enforcing this consistently.

To summarize the situation: there is in fact already a .editorconfig file in the root of the repo, mandating that indentation is done using spaces, but it is unclear whether Microchip Studio even supports .editorconfig to begin with.

The current state of mixed spacing rules throughout the firmware source code is likely a reflection of different people editing the code with different settings over time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New planned feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants